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

chore(deps-dev): bump webpack from 4.44.1 to 5.70.0 #11584

Merged
merged 7 commits into from
Mar 30, 2022
Merged

Conversation

AbdelrahmanHafez
Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez commented Mar 25, 2022

This PR upgrades webpack from v4.44.1 to v5.70.0.

The affecting breaking change is that, in webpack v4, Webpack used to polyfill node dependencies automatically which had performance drawbacks, in v5 webpack changed this to have people opt-in the dependencies they need for their applications.

Which means we need to install the following 3 dependencies:

  • assert (the browser version)
  • crypto-browserify
  • stream-browserify

My concern is regarding assert, it has a conflicting name with node's native assert, which means that we might get a slightly different behavior from node's assert.

Not sure if having assert as a dev dependency will mean that in production apps using mongoose will use the native assert or the browserify one, but even if production apps will use the native assert, another concern would be that we will have different behavior in the development process and CI will happily pass and we'd know about potential issues too late.

So it seems like our options are as follows:

  • Use node:assert, but that's only supported in v14 (only ESM), and v16 for CJS. So we can't do that now since we need to support Node 12.
  • Create an alias to assert-browserify that refers to npm:assert and use it.
  • Use a copy of npm:assert which is called assert-browserify, but it's not an official release, so I prefer not to go in this direction.
  • Avoid upgrading Webpack until we can use the node: protocol.

I'm leaning towards creating an alias to assert and calling it assert-browserify, and only use it for webpack.

What do you think?

Also, after upgrading Webpack started show warnings it didn't previously show regarding bundle size, any idea how we can decrease the bundle size?

Starting browser build...
asset ./dist/browser.umd.js 922 KiB [emitted] [minimized] [big] (name: main) 1 related asset
runtime modules 740 bytes 4 modules
modules by path ./node_modules/ 1.62 MiB
  javascript modules 1.6 MiB 204 modules
  json modules 12.7 KiB
    modules by path ./node_modules/browserify-sign/ 2.23 KiB
      ./node_modules/browserify-sign/browser/algorithms.json 2.07 KiB [built] [code generated]
      ./node_modules/browserify-sign/browser/curves.json 162 bytes [built] [code generated]
    + 4 modules
modules by path ./lib/ 638 KiB 145 modules
+ 11 modules

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets:
  ./dist/browser.umd.js (922 KiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (922 KiB)
      ./dist/browser.umd.js

WARNING in webpack performance recommendations:
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

webpack 5.70.0 compiled with 3 warnings in 11793 ms
Browser build successful.

@vkarpov15
Copy link
Collaborator

Honestly, I'm not sure it's worth our time to worry about upgrading Webpack currently. I'd like to upgrade in the future, but, like you said, there's some tradeoffs we need to consider to use Webpack 5. Is there any benefit to us upgrading?

@AbdelrahmanHafez
Copy link
Collaborator Author

The main benefit I am doing this for is using the latest features whenever we need to and making version upgrades cheaper once webpack v6 is released for example.

The other tradeoff is the assert issue which I solved by using a different name in package.json, which I verified mitigates my initial concern.

I also took a look at the v4.x bundle size vs v5.x bundle size. I was wrong when I said:

after upgrading Webpack started to show warnings it didn't previously show regarding bundle size

It did show the warning on v4.x, and v5.x's bundle is 40 KBs smaller (922 KBs vs 962 KBs).

I now think this should be good to go. What do you think?

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge this into 6.3 just so we err on the side of caution.

@vkarpov15 vkarpov15 changed the base branch from master to 6.3 March 30, 2022 20:08
@vkarpov15 vkarpov15 added this to the 6.3 milestone Mar 30, 2022
@vkarpov15 vkarpov15 merged commit e2b13b4 into 6.3 Mar 30, 2022
@AbdelrahmanHafez
Copy link
Collaborator Author

Happy to help 👍

@AbdelrahmanHafez AbdelrahmanHafez deleted the upgrade-webpack branch March 31, 2022 01:51
vkarpov15 added a commit that referenced this pull request Apr 27, 2022
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.

None yet

2 participants