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

Webpack 5 #7929

Closed
wants to merge 4 commits into from
Closed

Webpack 5 #7929

wants to merge 4 commits into from

Conversation

andriijas
Copy link
Contributor

This draft PR aims to integrate webpack 5 into create-react-app.
Integration is currently in early stages and not ready to for community testing yet as there are a couple of tasks to resolve before we have first successful builds. Thank you for your patience!

11/05/2019 Initial draft PR

Internal work

Description Commit
✔️ Make sure to use mode (we do since webpack 4)
✔️ Upgrade webpack to webpack 5 1ae2d5d
✔️ Remove futureEmitAssets 7eabe2a
✔️ Update IgnorePlugin arg sig 86b2200
⚠️ Adjust webpack to automatic node polyfill removal (rebase #7914) 426e6ce
🚧 Upgrade all plugins and loaders to latest version
🚧 Make sure build has no errors or warnings
🚧 Remove deprecation warnings during build
🚧 Update outdated options
🚧 Cleanup configuration

External work

Description Link
🚧 mini-css-extract-plugin compatibility webpack-contrib/mini-css-extract-plugin#458

@sebinsua
Copy link

sebinsua commented Mar 24, 2020

@andriijas Are you still working on this?

I created my own branch and did a lot more work so if you want to cherry-pick from this into here or anywhere feel free.

I'm able to run most of the e2e tests from tasks/local-test.sh (simple, kitchensink, kitchesink-eject, installs).

The one test suite which isn't working very well is behavior:

  1. Some of the plugins need to be updated. The snapshots contain messages which ModuleNotFoundPlugin and ModuleScopePlugin should produce and these aren't matching.
  2. fixtures/builds-with-multiple-runtimes/index.test.js is failing. I'm not sure what this is even testing...
  3. fixtures/typescript-typecheck/index.test.js is failing. This might be because fork-ts-checker-plugin hasn't been updated for Webpack 5 yet? It's currently not producing the error messages that are expected.
  4. html-webpack-plugin doesn't yet support Webpack 5. (This causes yarn start to fail on first run and when watching changes.)

Something really weird was that I had to amend this rule in order to avoid a validation error. And, perhaps related to this, but the default json-loader didn't work for me so I had to add one manually. (Edit: For some reason, removing either/both causes errors during the kitchensink E2E suite.)

@ScriptedAlchemy
Copy link

As one of the core maintainers of webpack. I’ll also be reviewing the progress here and will try to step in to support. Im about to finish next.js upgrade to WP5

@ScriptedAlchemy
Copy link

Require ensure has changed. So to get the same feature set, you likely want to either tap it from the JavaScriptGenerator. - I do believe a requireEnsure hook still exists though.

@andriijas
Copy link
Contributor Author

@sebinsua thanks for stepping in, I halted work on this by purpose - wanted webpack 5 to reach RC at least before continuing and for the obvious reason you mentioned, external plugins needs to be adressed with webpack 5 compability as well.

Im now thinking we should create a feature branch here to make smaller PRs to - would you be open to help out?

@piotr-oles
Copy link

Hi!
I'm the author of the fork-ts-checker-webpack-plugin. Just to let you know - I released an alpha version (v5.0.0-alpha.1) which supports Webpack 5 🚀

⚠️ Keep in mind that there are breaking changes regarding the configuration of the plugin - the configuration is documented in the README.md :)

@raix
Copy link
Contributor

raix commented Oct 10, 2020

Is this pull-request stale?
webpack/webpack#11406 (comment)

@andriijas
Copy link
Contributor Author

Is this pull-request stale?
webpack/webpack#11406 (comment)

Check comment here #9613 (comment)

@raix
Copy link
Contributor

raix commented Oct 11, 2020

@andriijas To test things out I ejected on one or our applications and managed to do yarn build I used npx serve to QA the build. (yarn start also works now...)

I'm not sure if it's helpful eg. to plan out smaller pull-requests for updating plugins etc.

Some findings updating to webpack 5

Loaders / Plugins

postcss-loader

  • Move config into "postcssOptions"
  • (converted plugins to an array instead of a lambda, not sure if that's the best way)

terser-webpack-plugin

  • no apparent changes was needed

ForkTsCheckerWebpackPlugin

new ForkTsCheckerWebpackPlugin({
    async: isEnvDevelopment,
    // TODO: Add back resolvers for pnpTs
    typescript: {
      typescriptPath: resolve.sync('typescript', {
        basedir: paths.appNodeModules,
      }),
      context: paths.appPath,
      diagnosticOptions: {
          syntactic: true
      },
      mode: 'write-references'
    },
    logger: {
      infrastructure: "silent"
    },
    // The formatter is invoked directly in WebpackDevServerUtils during development
    formatter: isEnvProduction ? typescriptFormatter : undefined,
  })

TODO: Add back pnp support

optimize-css-assets-webpack-plugin (optional afaik)

In babel-preset-react-app

ManifestPlugin

  • use 3.0.0-rc.0

webpack.IgnorePlugin

  • Use named options resourceRegExp and contextRegExp

WorkboxWebpackPlugin.GenerateSW

  • importWorkboxFrom deprecated (accordingly to the documentation no longer relevant, but would need to give it a reading)
  • navigateFallbackBlacklist -> navigateFallbackDenylist

References:

webpack 5 related:

In webpack.config.js:

Remove:

  • futureEmitAssets
  • jsonpFunction
  • { parser: { requireEnsure: false } }
  • node - this would be a breaking change?

Add this fix to the top

target: "browserslist",
// Fix babel resolve
// ref: https://github.com/webpack/webpack/issues/11467
{
    // Note: This is not the real fix, babel will release a fix in the upcoming week
    // Ref: https://github.com/babel/babel/issues/12058
    test: /\.m?js/,
    resolve: {
      fullySpecified: false,
    },
},

Fix filename in webpack.config.js

filename: isEnvProduction
        ? 'static/js/[name].[contenthash:8].js'
        : isEnvDevelopment && 'static/js/[name].js',

previously it was isEnvDevelopment && 'static/js/bundle.js' but got "Conflict: Multiple assets emit to the same filename" - there might be an underlying issue - eg. a double compilation etc.

In formatWebpackMessages.js

  • Remove chalk import
  • Remove usage of chalk at lines[0] = chalk.inverse(lines[0]);
  • Fix the format message input - not always a string
function formatMessage(messageObject) {
  let message = messageObject.message || messageObject;

In ModuleNotFoundPlugin.js

  • SingleEntryPlugin -> EntryPlugin

In ModuleScopePlugin.js

          // request.descriptionFileRoot.indexOf('/node_modules/') !== -1 ||
          // request.descriptionFileRoot.indexOf('\\node_modules\\') !== -1 ||
          request.path.includes('/node_modules/') ||
          request.path.includes('\\node_modules\\') ||

Looking at the request object the request.relativePath on my mac == './Users/...' - '.' + absolute path, request.path is wrong as it will be /Users/.../Users/... - seems like a webpack or babel issue - but the check still works

react-scripts build

Using npx serve on the static build the app starts up

react-scripts start

I've only just started looking at WebPackDevServerUtils.js only change I've done is:
getCompilerHooks(compiler) receive -> issues
Reference: TypeStrong/fork-ts-checker-webpack-plugin#490

Disclaimer - I just ejected to test things out - currently only looking into getting it running - next would be to verify each change and its correctness

@andriijas
Copy link
Contributor Author

@raix thx, ill might look into this during the week if i find some time :)

@ali-master
Copy link

Hi there,

This MR should be updated because the Webpack v5 has been released 3 days ago.

@kagawagao
Copy link

Compiler Stats updated in webpack 5, formatWebpackMessage should be updated

Stats json errors and warnings no longer contain strings but objects with information splitted into properties.
MIGRATION: Access the information on the properties. i. e. message

@andriijas
Copy link
Contributor Author

@andriijas @sebinsua are either of you still working on this?

Not since last update but I would like to pick it up soon,

@ScriptedAlchemy
Copy link

Once error formatting is done. I'll help on any remaining issues.

I'm not not dealing with the formatted haha

@jasonwilliams
Copy link
Contributor

Once error formatting is done. I'll help on any remaining issues.

I'm not not dealing with the formatted haha

@ScriptedAlchemy error formatting is easy, that snippet from @jimblue will solve it.
Maybe a new PR with all the changes from @sebinsua would be really helpful. I think we're close.

If you wanted to make that PR and leave errorFormatting i can even PR that in.

@ScriptedAlchemy
Copy link

Where the most current branch, or do we need one which aggregates two branch change sets together?

Just wrapping up storybooks webpack 5 upgrade, then can pull whatever branch is most current

@andriijas
Copy link
Contributor Author

Where the most current branch, or do we need one which aggregates two branch change sets together?

Just wrapping up storybooks webpack 5 upgrade, then can pull whatever branch is most current

The fastest option is to create a new PR from scratch if you need to move quickly.

@ScriptedAlchemy
Copy link

I don't use CRA, so I'm just here to unstick any problems blocking the community on the nearest progressed branch. Will look around and see what's been proposed already

@raix
Copy link
Contributor

raix commented Nov 2, 2020

@ScriptedAlchemy Let me know if you need help on making a pr - I do have a branch working locally with webpack 5 (when doing a manual QA), continued on the branch @sebinsua created on duplotech. (but it would be nice to break it up into smaller pieces or redo it)

That said a strategy could also be to make small pull-requests to master upgrading the different plugins.

  • The ManifestPlugin plugin is slowly getting up to speed / maintained again (haven't seen a wp5 version yet)
  • The Webpack dev server doesn't yet support federated modules so we'll likely have to wait abit on that.
  • Not sure if CRA have swapped to use the ESLintPlugin instead of eslint-loader? (Edit: fixed in Replace deprecated eslint-loader by eslint-webpack-plugin #9751 )
  • OptimizeCSSAssetsPlugin should likely swap to use css-minimizer-webpack-plugin instead.
  • Webpack 5 dropped nodejs builtin package shims - not sure if we need to still support this. (packages like axios still need this afaik)
  • I did also update postcss having some changes - not sure if needed.
  • I didn't check the PnpWebpackPlugin if that works (we currently don't use PnP - and the tests on CRA are broken, not sure if the feature actually still works)
  • IgnorePlugin and GenerateSW changed signature - minor change
  • ForkTsCheckerWebpackPlugin update contains more changes also renaming the hook to issues (as I remember)
  • And yes... fix the format webpack messages (did it in one line to get it working - on both WP4 and 5 - but would require more work)
  • The webpack config it self has minor changes, a filename to be updated etc. (CRA would then be WP5 only?)
  • And omg fix the test suite and setup policies on the pipeline to block pr's with failing tests...please (I have one pr to fix / improve part of the test suite - but there are more issues)

More findings in #7929 (comment)

(I have to move really fast so did actually build a CRA compatible build from scratch with federated module support written in TypeScript - but would prefer to use CRA if possible, I'm hoping to move back at some point - will help out if needed)

Kind regards Morten

@andriijas
Copy link
Contributor Author

@raix Thanks for the compilation. I agree it would be beneficial to try to cut the upgrade to as small chunks as possible. Getting support for federated modules should be done separately as it's a new feature however it will be useful for large multi team companies who use CRA for microfrontend dev.

@jasonwilliams
Copy link
Contributor

Agree with both small pull requests and doing module federation separately.

I wouldn’t focus on module federation right now as no one is waiting on that and can be done at a later date. We don’t want to add even more scope to this.

@raix can I suggest we move your list to an issue? Then we can keep the list as PRs get merged.

@raix raix mentioned this pull request Nov 2, 2020
25 tasks
@raix
Copy link
Contributor

raix commented Nov 2, 2020

@jasonwilliams @andriijas I've created an issue containing an initial overview of what needs to be tackled to get Webpack 5 in CRA - #9994 let me know what you think

@jasonwilliams
Copy link
Contributor

Looks good to me, I reckon we can close this PR and move conversation there

sapegin pushed a commit to styleguidist/react-styleguidist that referenced this pull request Nov 5, 2020
Refs #1703

* Inline loaders and ! prefixes should not be used as they are non-standard. They may be used by loader generated code.
https://webpack.js.org/configuration/module/#ruleenforce
* Adds support for both Webpack 4 and 5 in StyleguidistOptionsPlugin
* Updates dependencies in Webpack example (easier to test against)
* Because automatic polyfilling is switched off in 5, assert was brought in as a dependency (it's used by Doctrine, see below)

## Upstream issues

Both issues below are tied to facebook/create-react-app#7929:

* Process is not defined - The page still builds but this error will show in the console. I can't seem to polyfill this if anyone else can that would be great, then I think we're done.
* TypeError: message.split is not a function - you may not get this error, but if you do it's related to facebook/create-react-app#7929. The quick fix is to go into node_modules/react-dev-utils/formatWebpackMessages.js:19 and add the code from facebook/create-react-app#7929 (comment)

## Not needed for this but nice to have.

Doctrine should be replaced with another JSDoc parser. Doctrine is end of life and no longer supported. It causes problems with Webpack 5 because it pulls in assert which WP5 does not polyfill. For now, we can fix it by adding those polyfills (assert) but a more stable solution should be found. The issue raised: #1708
@raix
Copy link
Contributor

raix commented Dec 4, 2020

just a minor update:
We are down to creating 3 tiny pull-requests and have 6 pending pull-requests #9994
The final pull-request will be a fairly small change as it's only updating terser and webpack specific config changes.

@andriijas
Copy link
Contributor Author

@raix ive seen it, i will help review and testing. Good work!

@andriijas andriijas closed this Dec 4, 2020
@raix
Copy link
Contributor

raix commented Dec 4, 2020

@andriijas thank you! I'll keep the status updated and in general help to make progress

But if possible it might make sense to prioritize merging #10091 (and #10177) ? as it will fix part of the test suite - would be really helpful, I normally fix the test suite locally and run tests but it's a slow process
I also suggested to skip the rest of failing tests for now and only merge when tests are all green, maybe enforced by a policy if possible

@cristovaoth
Copy link

@ScriptedAlchemy Let me know if you need help on making a pr - I do have a branch working locally with webpack 5 (when doing a manual QA), continued on the branch @sebinsua created on duplotech. (but it would be nice to break it up into smaller pieces or redo it)

That said a strategy could also be to make small pull-requests to master upgrading the different plugins.

  • The ManifestPlugin plugin is slowly getting up to speed / maintained again (haven't seen a wp5 version yet)
  • The Webpack dev server doesn't yet support federated modules so we'll likely have to wait abit on that.
  • Not sure if CRA have swapped to use the ESLintPlugin instead of eslint-loader? (Edit: fixed in Replace deprecated eslint-loader by eslint-webpack-plugin #9751 )
  • OptimizeCSSAssetsPlugin should likely swap to use css-minimizer-webpack-plugin instead.
  • Webpack 5 dropped nodejs builtin package shims - not sure if we need to still support this. (packages like axios still need this afaik)
  • I did also update postcss having some changes - not sure if needed.
  • I didn't check the PnpWebpackPlugin if that works (we currently don't use PnP - and the tests on CRA are broken, not sure if the feature actually still works)
  • IgnorePlugin and GenerateSW changed signature - minor change
  • ForkTsCheckerWebpackPlugin update contains more changes also renaming the hook to issues (as I remember)
  • And yes... fix the format webpack messages (did it in one line to get it working - on both WP4 and 5 - but would require more work)
  • The webpack config it self has minor changes, a filename to be updated etc. (CRA would then be WP5 only?)
  • And omg fix the test suite and setup policies on the pipeline to block pr's with failing tests...please (I have one pr to fix / improve part of the test suite - but there are more issues)

More findings in #7929 (comment)

(I have to move really fast so did actually build a CRA compatible build from scratch with federated module support written in TypeScript - but would prefer to use CRA if possible, I'm hoping to move back at some point - will help out if needed)

Kind regards Morten

We are trying to achieve the same, i.e., eject latest cra, and upgrade to webpack@5. We followed all the steps you outlined above, and also under #9994, but currently not able to make it work.

Perhaps you could push the aforementioned local branch where you made this work @raix ? That would be super helpful

Kind regards

@raix
Copy link
Contributor

raix commented Jan 5, 2021

@CristovaoHonorato I dont have that work locally anymore (I clean up stale work, was a POC, result of that week was custom wp/cli and the key points posted here) - the pull-requests open in the CRA project should bring remaining changes down to the webpack config (The branch @sebinsua created might also give clues to changes needed in the config)

mbarrien added a commit to chanzuckerberg/czgenepi that referenced this pull request Feb 9, 2021
Needed for future Docker compatibility.

Manually applied many of the PRs from facebook/create-react-app#9994
and facebook/create-react-app#7929 (including comments)
mbarrien added a commit to chanzuckerberg/czgenepi that referenced this pull request Feb 9, 2021
Needed for future Docker compatibility.

Manually applied many of the PRs from facebook/create-react-app#9994
and facebook/create-react-app#7929 (including comments)
mbarrien added a commit to chanzuckerberg/czgenepi that referenced this pull request Feb 10, 2021
Needed for future Docker compatibility.

Manually applied many of the PRs from facebook/create-react-app#9994
and facebook/create-react-app#7929 (including comments)
@D4nte
Copy link

D4nte commented Aug 17, 2021

Is this included in 5.0.0-next.31?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.