Skip to content

No new apps should be started with sprockets#52887

Merged
dhh merged 10 commits intomainfrom
propshaft-only-for-new-apps
Sep 14, 2024
Merged

No new apps should be started with sprockets#52887
dhh merged 10 commits intomainfrom
propshaft-only-for-new-apps

Conversation

@dhh
Copy link
Member

@dhh dhh commented Sep 11, 2024

So remove the -a option and all generated code for sprockets. Sprockets will remain an option for existing applications, though.

@rails-bot rails-bot bot added the railties label Sep 11, 2024
@dhh dhh added this to the 8.0.0 milestone Sep 11, 2024
@dhh
Copy link
Member Author

dhh commented Sep 11, 2024

Would be great if anyone is interested in carrying this to completion with whatever test updates it needs.

@andrewn617
Copy link
Member

@dhh I have a few commits that I think fix the test issues. I can't push to this repo tho so I can't push on your branch.

propshaft-only-for-new-apps...Shopify:rails:propshaft-only-for-new-apps

Feel free to cherry pick them, or if you'd rather I do it I can open a new from my fork.

* Use propshaft in abstract unit tests

* Fix call to asset_pipeline option in application template

---------

Co-authored-by: Andrew Novoselac <andrew.novoselac@shopify.com>
@dhh
Copy link
Member Author

dhh commented Sep 12, 2024

Thanks @andrewn617. Just made a PR and merged in 👍

@andrewn617
Copy link
Member

Looks like my change causes a lot of assets_test.rb to fail since it has sprockets behaviour tested, maybe some others too. I can try to take a look at that later.

@zzak
Copy link
Member

zzak commented Sep 12, 2024

👋 I was spiking to get the build green over here, but wasn't going to ping until it was fixed.

I was also trying to incorporate #52122, once things are green will take a closer look at the changes.

* main:
  Skip solid_cable gem if skip_action_cable is set (#52919)
  Call prerecord in test parallelization
  Fix a documentation name mismatch in cookies.rb
  Revert "Assign id attributes first in Active Record attribute assignment"
  Do not populate CurrentAttributes#attributes when not using defaults
  Suppress a RuboCop offense after running `rails new` with `--devcontainer`
  Fix small typo for Solid Cable changelog [ci skip] (#52911)
  Use less queries when updating nested attributes
  Load routes in LazyRoutesSet#recognize_path
  Slim down the gem section for Solid
  Add Solid Cable (#52889)
  Fix syntax error in user.rb
  Improve Active Record Association Callbacks docs [ci skip]
  Assign id attributes first in Active Record attribute assignment
  Always separate config blocks with a CR
  Fixup tests for #52850
  Replace "healthcheck" with "health check" in documentation. [ci-skip]
…52899)

Create `sprockets_assets_test.rb` file that will contain the
sprockets-specific tests of `assets_test.rb` (which in turn will contain
the tests common to both pipelines).

There is one flaky test, but I have seen this flake on main before.

```
$ bin/test test/application/sprockets_assets_test.rb

........................E, [2024-09-14T08:35:08.072799 #1983325] ERROR -- : [1ae49fc5-8c5a-48e4-ab4b-0ce5e73f79d0]
[1ae49fc5-8c5a-48e4-ab4b-0ce5e73f79d0] ActionController::RoutingError (No route matches [GET] "/assets/demo.js"):
[1ae49fc5-8c5a-48e4-ab4b-0ce5e73f79d0]
..** Invoke assets:precompile (first_time)
** Invoke assets:environment (first_time)
** Execute assets:environment
** Invoke environment (first_time)
** Execute environment
** Execute assets:precompile
F

Failure:
ApplicationTests::SprocketsAssetsTest#test_precompile_shouldn't_use_the_digests_present_in_manifest.json [test/application/sprockets_assets_test.rb:326]:
Expected "application-0a89120b13dcff9cb069311101add34ef7717de154816b01622972a4509eb5c9.css" to not be equal to "application-0a89120b13dcff9cb069311101add34ef7717de154816b01622972a4509eb5c9.css".

bin/test test/application/sprockets_assets_test.rb:307

..

Finished in 1.424157s, 20.3629 runs/s, 120.0710 assertions/s.
29 runs, 171 assertions, 1 failures, 0 errors, 0 skips

$ bin/test test/application/sprockets_assets_test.rb

........................E, [2024-09-14T08:35:12.994864 #1985849] ERROR -- : [615c9d49-d1bf-40c8-96a6-bf9c76a9a28d]
[615c9d49-d1bf-40c8-96a6-bf9c76a9a28d] ActionController::RoutingError (No route matches [GET] "/assets/demo.js"):
[615c9d49-d1bf-40c8-96a6-bf9c76a9a28d]
..** Invoke assets:precompile (first_time)
** Invoke assets:environment (first_time)
** Execute assets:environment
** Invoke environment (first_time)
** Execute environment
** Execute assets:precompile
I, [2024-09-14T08:35:13.044219 #1986490]  INFO -- : Writing /home/zzak/code/rails/tmp/d20240914-1985782-wcigrx/app/public/assets/rails-6738e33efa7b8d2036e0a0118601555f0b771ac042f6790f7538dd881a1a7f3a.png
I, [2024-09-14T08:35:13.045199 #1986490]  INFO -- : Writing /home/zzak/code/rails/tmp/d20240914-1985782-wcigrx/app/public/assets/application-f1fa81296280307c358b4bd65e4bfd705be90dc5eba7c04dcd1d76ec084c78e7.css
I, [2024-09-14T08:35:13.045508 #1986490]  INFO -- : Writing /home/zzak/code/rails/tmp/d20240914-1985782-wcigrx/app/public/assets/application-f1fa81296280307c358b4bd65e4bfd705be90dc5eba7c04dcd1d76ec084c78e7.css.gz
I, [2024-09-14T08:35:13.045767 #1986490]  INFO -- : Writing /home/zzak/code/rails/tmp/d20240914-1985782-wcigrx/app/public/assets/rails-6738e33efa7b8d2036e0a0118601555f0b771ac042f6790f7538dd881a1a7f3a.png
...

```

Co-authored-by: Tute Costa <tutecosta@gmail.com>
Co-authored-by: Federico Sapuppo <fedesapuppo@hotmail.com>
@dhh dhh merged commit 0f43fed into main Sep 14, 2024
@dhh dhh deleted the propshaft-only-for-new-apps branch September 14, 2024 04:00
dhh added a commit that referenced this pull request Sep 14, 2024
* main:
  No new apps should be started with sprockets (#52887)
  Skip solid_cable gem if skip_action_cable is set (#52919)
  Call prerecord in test parallelization
  Fix a documentation name mismatch in cookies.rb
  Revert "Assign id attributes first in Active Record attribute assignment"
  Do not populate CurrentAttributes#attributes when not using defaults
  Suppress a RuboCop offense after running `rails new` with `--devcontainer`
DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
* No new apps should be started with sprockets

Co-authored-by: Andrew Novoselac <andrew.novoselac@shopify.com>
Co-authored-by: Tute Costa <tutecosta@gmail.com>
Co-authored-by: Federico Sapuppo <fedesapuppo@hotmail.com>
Co-authored-by: zzak <zzakscott@gmail.com>
Co-authored-by: Federico Sapuppo <fedesapuppo@hotmail.com>
Earlopain added a commit to Earlopain/rails that referenced this pull request Nov 8, 2024
* rails#51831
* rails#51342
* rails#52887

Probably more, these are the ones I noticed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants