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

🔥 Remove webpacker #3848

Merged
merged 3 commits into from
Oct 4, 2024
Merged

🔥 Remove webpacker #3848

merged 3 commits into from
Oct 4, 2024

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Jul 16, 2024

@josemigallas josemigallas self-assigned this Jul 16, 2024
@akostadinov
Copy link
Contributor

Removing webpacker easier than upgrading it, you are full of surprises. It is cool when it is about removing.

@josemigallas josemigallas force-pushed the bye_bye_webpacker branch 3 times, most recently from 895708f to f482902 Compare August 1, 2024 14:43
@josemigallas josemigallas changed the base branch from master to ruby-3.1 August 6, 2024 06:26
.circleci/config.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a better look to this tomorrow, but for now my question is: why not switching to jsbundling-rails instead of this? It's the official replacement for Webpacker in Rails 7 (announcement)

Guide: https://github.com/rails/jsbundling-rails/blob/main/docs/switch_from_webpacker.md

What I don't like about this approach is we might be inventing the wheel and implementing a homebrew solution, this has several problems. For instance, having to maintain it. A gem like webpacker gets updated, the CVEs are solved, etc. In this case we should do all of this ourselves, and we probably won't. Also it might require some extra work when we want to upgrade Rails.

Copy link
Contributor

@jlledom jlledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine for me locally. Good job. A few comments though:

  • I see many compile warnings about conflicting order, why is that?
  • I tried to run this branch with RAILS_ENV=production and I can't load any screen because the file errors.css is being requested as mimetype 'text/css' but the servers returns it as mimetype text/html, which causes and error 500 for some reason.

.browserslistrc Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
app/helpers/webpack_helper.rb Show resolved Hide resolved
config/examples/settings.yml Outdated Show resolved Hide resolved
lib/tasks/webpack.rake Outdated Show resolved Hide resolved
test/unit/liquid/filters/rails_helpers_test.rb Outdated Show resolved Hide resolved
lib/tasks/assets.rake Outdated Show resolved Hide resolved
@josemigallas josemigallas force-pushed the bye_bye_webpacker branch 3 times, most recently from e7874fb to 5ea1092 Compare September 4, 2024 14:33
@josemigallas josemigallas marked this pull request as ready for review September 4, 2024 15:03
jlledom
jlledom previously approved these changes Sep 19, 2024
lvillen
lvillen previously approved these changes Sep 20, 2024
Copy link
Contributor

@lvillen lvillen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments which aren't blockers. Besides that, I'm excited with the changes introduced here! 🚀

@mayorova
Copy link
Contributor

I found some pages where the styles don't look good while running locally with bundle exec rails assets:precompile (not the webpack dev server)
Screenshot from 2024-09-20 10-54-55
Screenshot from 2024-09-20 10-56-10
Screenshot from 2024-09-20 10-56-31
Screenshot from 2024-09-20 10-57-26
Screenshot from 2024-09-20 10-57-38
Screenshot from 2024-09-20 10-58-32
Screenshot from 2024-09-20 10-59-02
Screenshot from 2024-09-20 10-59-10
Screenshot from 2024-09-20 10-59-28
Screenshot from 2024-09-20 10-59-36
Screenshot from 2024-09-20 11-00-26
Screenshot from 2024-09-20 11-00-42

@lvillen
Copy link
Contributor

lvillen commented Sep 20, 2024

I found some pages where the styles don't look good while running locally with bundle exec rails assets:precompile (not the webpack dev server)

I've been testing it after compiling assets running bundle exec rails webpack:dev and it looks good.

@josemigallas josemigallas dismissed stale reviews from lvillen and jlledom via e8970d8 September 27, 2024 12:23
@josemigallas josemigallas force-pushed the bye_bye_webpacker branch 2 times, most recently from e8970d8 to d47b85d Compare October 3, 2024 10:27
@josemigallas josemigallas force-pushed the bye_bye_webpacker branch 3 times, most recently from 1d21349 to 4b8b566 Compare October 3, 2024 12:33
@josemigallas josemigallas changed the title 🚧 Remove webpacker 🔥 Remove webpacker Oct 3, 2024
@lvillen
Copy link
Contributor

lvillen commented Oct 3, 2024

I've been checking @mayorova pages that were failing in production earlier, and the only one still failing is the SSO Integrations edit form, which fails both in development and production.

I'd say app/views/provider/admin/authentication_providers/edit.html.slim is missing the

- content_for :javascripts do
  = stylesheet_packs_chunks_tag 'pf_form'

Besides that, both development and production work fine.

Copy link
Contributor

@mayorova mayorova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! 🔥

Stellar job, @josemigallas 🌟

@josemigallas josemigallas merged commit 91acc28 into master Oct 4, 2024
1 of 10 checks passed
@josemigallas josemigallas deleted the bye_bye_webpacker branch October 4, 2024 11:05
@josemigallas josemigallas mentioned this pull request Oct 7, 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.

5 participants