spring icon indicating copy to clipboard operation
spring copied to clipboard

Fix frozen string literal for ruby 3.4

Open chaadow opened this issue 1 year ago • 5 comments

After running RUBYOPT='--enable-frozen-string-literal --debug-frozen-string-literal' bin/rails c ( Following this guide )

I had this issue:

/Users/chedli/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/spring-4.2.1/lib/spring/json.rb:367:in `unquote': can't modify frozen String: "", created at /Users/chedli/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/spring-4.2.1/lib/spring/json.rb:367 

I went ahead and updated github actions as well, inspired by this https://github.com/svenfuchs/rails-i18n/pull/1120/

chaadow avatar May 24 '24 12:05 chaadow

I think I need help, for some reason the vendored JSON library is not called in acceptance tests? and I know for sure it does a string mutation because my application warns ( and crashes because i've monkey patch ruby's warn method to raise an error )

chaadow avatar Jun 10 '24 15:06 chaadow

@chaadow I think the way to fix this would be to run okjson's tests with frozen string literals enabled, then re-embed it in to Spring. It looks like the upstream repo has been archived though, so I'm not sure who is in charge of this code anymore.

tenderlove avatar Jun 17 '24 22:06 tenderlove

FWIW I ran the OKJson tests using your patch, and that fixed all of the errors there so I think this change is fine. Can you also add the "frozen string literal" directive to the top of json.rb? Thanks!

tenderlove avatar Jun 17 '24 22:06 tenderlove

@tenderlove Done! thanks for the review. I do think this vendored JSON is outdated and can be switched with a more recent implementation.

The CI is failing, but I can't figure out why in the errors displayed, if you have any tips please let me know.

I'll keep trying to investigate when I got more time.

chaadow avatar Jun 18 '24 09:06 chaadow

@tenderlove Based on this ( https://github.com/rails/spring/pull/713) the PR was merged even though the CI was failing.

and the CI errors are similar to here.

So maybe, if you don't mind, we can merge this PR and repair CI in another PR?

chaadow avatar Jun 23 '24 11:06 chaadow

Hi @tenderlove is it possible to get a last review of this PR please? 🙏🏼

chaadow avatar Oct 20 '24 12:10 chaadow