Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Use propshaft instead of sprockets #3470

Closed
wants to merge 5 commits into from

Conversation

segiddins
Copy link
Member

No description provided.

@segiddins segiddins marked this pull request as draft February 20, 2023 19:27
@codecov
Copy link

codecov bot commented Feb 20, 2023

Codecov Report

Merging #3470 (56f4603) into master (e5dee48) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3470   +/-   ##
=======================================
  Coverage   97.80%   97.80%           
=======================================
  Files         166      166           
  Lines        4145     4145           
=======================================
  Hits         4054     4054           
  Misses         91       91           
Impacted Files Coverage Δ
app/helpers/application_helper.rb 97.05% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@indirect
Copy link
Member

indirect commented Feb 21, 2023

I like the idea of switching to propshaft! I don't really feel like the 5 npm packages we use justify using esbuild. Can we use importmap-rails and skip the entire JS build step, removing esbuild and jsbundling entirely?

Related, since we don't seem to actually have any Sass, can we switch to completely vanilla CSS files (with regular-ass @import statements if needed), and completely skip the Sass step?

If we do both of those things, we should be able to

  1. avoid Node entirely for our assets and,
  2. still use and update npm packages for JS,

which is (in my opinion) the main selling point of propshaft vs sprockets.

@hsbt
Copy link
Member

hsbt commented Feb 21, 2023

Related, since we don't seem to actually have any Sass, can we switch to completely vanilla CSS files (with regular-ass @import statements if needed), and completely skip the Sass step?

or to use dartsass-rails and importmap-rails?

I have no strong opinion about jsbundling-rails/cssbundling-rails vs importmap-rails/dartsass-rails(or propshaft with sass).

@indirect
Copy link
Member

Oh yeah, if we really need Sass we can use dartsass-rails and also skip Node 👍🏻 But if I'm reading this diff right, we don't currently use Sass, so I think we can get away with just plain CSS and no CSS compiler at all.

@simi
Copy link
Member

simi commented Feb 22, 2023

The only sass file is https://github.com/rubygems/rubygems.org/blob/master/vendor/assets/stylesheets/github_buttons.css.scss, but that's actually css file if I understand it well.

I think the simple assets pipeline, same as generated with rails new myapp -a propshaft --skip-hotwire, will be enough. No need for compress, transpile, terser, ... Performance could be handled by HTTP/2 and gzip. We just need to ensure majority of browser accesses are able to continue using the page as is. IMHO that's ok since the CSS/JS features used currently are very conservative for 2023.

On the other side propshaft is new and having dev problems like rails/propshaft#90. 🤔

@simi simi mentioned this pull request Oct 7, 2023
@segiddins segiddins mentioned this pull request Oct 10, 2023
@simi simi mentioned this pull request Jan 25, 2024
@indirect
Copy link
Member

Closing this in favor of #4123.

@indirect indirect closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants