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

Upgrade to webpacker/webpack 4 and Babel 7 #6664

Merged
merged 16 commits into from Mar 17, 2020

Conversation

@nickytonline
Copy link
Member

nickytonline commented Mar 16, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Frontend infrastructure has been updated. We are now running on Babel 7 and webpacker/webpack 4.

This removes a blocker for upgrading to Rails 6 (webpacker 4) and will also allow us to upgrade to the latest Storybook.

Note: I will update Storybook in another PR, so it will temporarily be broken.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added tests?

  • yes
  • no, because they aren't needed
  • [] no, because I need help

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

Starlord from Guardians of the Galaxy saying "Finally"

@pr-triage pr-triage bot added the PR: draft label Mar 16, 2020
@nickytonline nickytonline removed the PR: draft label Mar 16, 2020
@nickytonline nickytonline marked this pull request as ready for review Mar 16, 2020
@nickytonline nickytonline requested review from thepracticaldev/hells-bells as code owners Mar 16, 2020
@nickytonline nickytonline requested review from , fdoxyz and maestromac and removed request for thepracticaldev/hells-bells Mar 16, 2020
@pr-triage pr-triage bot added the PR: unreviewed label Mar 16, 2020
@nickytonline nickytonline requested a review from jacobherrington as a code owner Mar 16, 2020
@nickytonline nickytonline requested review from rhymes and benhalpern and removed request for , fdoxyz and jacobherrington Mar 16, 2020
@@ -47,7 +47,7 @@
<button for="subscribe_notifications" id="notification-subscription-label_only_author_comments" class="notification-subscription-label" data-payload="only_author_comments">
Post Author Comments <span class="selected-emoji"></span>
</button>
<%= javascript_pack_tag "notificationSubscriptionHandler", defer: true %>
<%= javascript_packs_with_chunks_tag "notificationSubscriptionHandler", defer: true %>

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

This is the new webpacker helper function. For more information, see https://github.com/rails/webpacker/blob/master/lib/webpacker/helper.rb

@@ -28,6 +28,5 @@
<% end %>

<%= render "shared/webcomponents_loader_script" %>
<%= javascript_pack_tag "clipboardCopy", defer: true %>
<%= javascript_pack_tag "articleForm", defer: true %>
<%= javascript_packs_with_chunks_tag "clipboardCopy", "articleForm", defer: true %>

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

All packs on a page need to be included in the new helper. This prevents chunks from loading multiple times. For more information, see the webpacker 4 upgrade guide.

@@ -45,7 +45,9 @@
<% end %>
</div>
</div>
<%= javascript_pack_tag "listingForm", defer: true %>
<% unless @organizations.present? %>

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

To avoid the issue mentioned above about chunks loading multiple times, I have to check if the user is logged on or not. If they aren't, just load the "listingForm" pack. Otherwise, load the "listingForm" and "orgCreditSelector" packs within the same javascript_packs_with_chunks_tag call.

I do this in a few other places below.

This comment has been minimized.

Copy link
@rhymes

rhymes Mar 16, 2020

Contributor

You wrote you need to check the user logged in status, but doesn't this check @organizations ?
I'm asking as I don't think there's a way to get to this page if you're not signed in:

before_action :authenticate_user!, only: %i[edit update new dashboard]

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

Ooops, I meant @organizations. 🙃

}),
);
// Enable the default config
environment.splitChunks();

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

The new way to split chunks.

@@ -3,26 +3,47 @@
default: &default
source_path: app/javascript
source_entry_path: packs
public_root_path: public

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

Part of the webpacker upgrade is to merge the current webpacker.yml file with this one.

"chart.js": "^2.9.3",
"clipboard-polyfill": "^2.8.6",
"codemirror": "^5.52.0",
"core-js": "3",

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

Babel 6 shipped with core-js 2 out of the box. Now you need to explicitly install core-js and reference it in babel.config.js.

@@ -195,7 +195,7 @@ preact-render-spy (1 nodes)

<button
type="button"
onClick={[Function value]}
onClick={[Function anonymous]}

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 16, 2020

Author Member

I'll be honest, I do not know why this changed to anonymous here and below in a bunch of other snapshot tests.

This comment has been minimized.

Copy link
@maestromac

maestromac Mar 16, 2020

Member

My guess is the newer version of babel 🤷‍♂ . Happened to me too every single time I tried to upgrade Webpacker.

This comment has been minimized.

Copy link
@nickytonline

nickytonline Mar 17, 2020

Author Member

Same. 🙃

Copy link
Member

maestromac left a comment

I'm so excited for this! Tested locally, LGTM 🙌

also, would you like to temporarily comment-out/remove yarn build-storybook from our .travis.yml until the new storybook is in place?

config/webpack/environment.js Show resolved Hide resolved
@nickytonline nickytonline requested a review from thepracticaldev/sre as a code owner Mar 17, 2020
@rhymes
rhymes approved these changes Mar 17, 2020
Copy link
Contributor

rhymes left a comment

Yes!

@nickytonline

This comment has been minimized.

Copy link
Member Author

nickytonline commented Mar 17, 2020

A question: would we ever need stylesheet_packs_with_chunks_tag and favicon_pack_tag ? I guess the underlying question is: would we move the other assets from sprockets to webpack?

@rhymes, good question. We could move other assets from sprockets to webpack. For stylesheets, they'd be inlined and hot reloaded during development and for production, it would generate actual CSS files via the MiniCSSExtract plugin. If that is not set up in our webpack config it could be added fairly easily.

For the fav_icon_pack_tag, I'm not sure what the advantages of using it are. I wasn't aware of this helper.

There also have been projects that have completely moved away from Sprockets, but in our case, I don't think that would be possible at the moment and I'm not even sure we'd want to do that.

@rhymes

This comment has been minimized.

Copy link
Contributor

rhymes commented Mar 17, 2020

Thanks for the great explanation. The good thing is that we can experiment and see what makes sense to be moved to Webpack and what not. We do have a lot of JS that's outside Webpack as well

@nickytonline nickytonline merged commit ccf7e6e into master Mar 17, 2020
8 checks passed
8 checks passed
Gitpod Open an online workspace in Gitpod
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (50% threshold)
Details
codeclimate/total-coverage 93% (0.0% change)
Details
deploy/netlify Deploy preview ready!
Details
license/cla Contributor License Agreement is signed.
Details
@pr-triage pr-triage bot removed the PR: unreviewed label Mar 17, 2020
@nickytonline nickytonline deleted the nickytonline/webpacker-webpack-4-babel7-upgrade branch Mar 17, 2020
@pr-triage pr-triage bot added the PR: merged label Mar 17, 2020
@nickytonline nickytonline mentioned this pull request Mar 21, 2020
3 of 11 tasks complete
vaidehijoshi added a commit to vaidehijoshi/dev.to that referenced this pull request Mar 23, 2020
Upgraded to webpacker 4/webpack 4 and Babel 7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.