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

support Webpack 5 #1707

Merged
merged 11 commits into from
Nov 5, 2020
Merged

Conversation

jasonwilliams
Copy link
Collaborator

@jasonwilliams jasonwilliams commented Oct 30, 2020

The changes below still support Webpack v4. Styleguidist itself still uses webpack 4 but this adds support for projects using Webpack 5. Fixes #1703

From my testing it does work fine for both versions, there are some upstream issues but i don't see those blocking this from going in.

  • Inline loaders and ! prefixes should not be used as they are non-standard. They may be use 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)
  • Some types needed changing

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 its related to Webpack 5 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 Webpack 5 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. Issue raised: Replace doctrine with another JSDoc parser #1708

How to test

  • Checkout this branch
  • Change webpack to version 5 in the package.json (this is so that during runtime the correct version of webpack is used, otherwise because of yarn link it will end up using styleguidists webpack version and not the example's version)
  • yarn link this project into examples/webpack
  • staying at the top level run rm -rf lib && npm install && npm run-script compile
  • Go into examples/webpack and run yarn styleguide

@jasonwilliams jasonwilliams changed the title Update loader syntax to support Webpack 5 support Webpack 5 Oct 31, 2020
@codecov
Copy link

codecov bot commented Nov 1, 2020

Codecov Report

Merging #1707 into master will not change coverage.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
styleguide.config.js 100.00% <ø> (ø)
test/apps/basic/styleguide.config.js 100.00% <ø> (ø)
test/data/styleguide.config.js 100.00% <ø> (ø)
src/scripts/utils/StyleguidistOptionsPlugin.ts 100.00% <100.00%> (ø)

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

Would it make sense to stop using create-react-app’s scripts? I doubt that supporting multiple versions of webpack is their goal, and we may end up with a dependency that only works with webpack 4 or 5.

examples/webpack/package.json Outdated Show resolved Hide resolved
@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Nov 3, 2020

Thanks for the pull request!

Would it make sense to stop using create-react-app’s scripts? I doubt that supporting multiple versions of webpack is their goal, and we may end up with a dependency that only works with webpack 4 or 5.

I don't know what half those scripts do so i can't really answer that, but formatWebpackMessages could be brought into this app as it's just one function. That would only unblock one of the issues though.

There is plan for react-dev-util to support webpack 5 here facebook/create-react-app#9994 i don't know if they will drop 4 or support both 4 and 5.

@sapegin
Copy link
Member

sapegin commented Nov 3, 2020

There is plan for react-dev-util to support webpack 5 here facebook/create-react-app#9994 i don't know if they will drop 4 or support both 4 and 5.

They bundle webpack so there's no reason for them to support multiple versions.

@jasonwilliams
Copy link
Collaborator Author

@sapegin I’m not sure what you want me to do about that. Replacing the functionality of React-dev-utils is far out of scope for me as I don’t know what it does and I doubt I could rebuild it all in this repo.

How they add functionality to 5 I don’t know if that means 4 will no longer work, they bundle webpack but that doesn’t neccesarily mean it can’t integrate with 4, I suppose we just wait and see for that.

In terms of this PR, everything needed here is done, the only remaining problems are not here so this could go ahead in theory.

@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Nov 3, 2020

@sapegin is there a reason you need to keep supporting 4? Other plugins and deps are dropping 4 going forward. Webpack themselves no longer support 4. So my guess is any user of this will just lock to a version if they can’t upgrade yet.

@sapegin
Copy link
Member

sapegin commented Nov 3, 2020

There are three things we use from react-dev-utils:

And we could probably use more. Maintaining them all inside Styleguidist isn't an option, there's a lot of tricky code, that will break, and someone will have to fix it. Better to rely on a popular project for that.

I’m not sure what you want me to do about that

This is up to you, if this pull request isn't breaking anything for webpack 4 right now, we can merge it as is, and it will be a huge step forward in supporting webpack 5.

is there a reason you need to keep supporting 4? Other plugins and deps are dropping 4 going forward. Webpack themselves no longer support 4. So my guess is any user of this will just lock to a version if they can’t upgrade yet.

The experience of working with real projects is my reason. It will take years to migrate to webpack 5. And migrations, when you have to upgrade multiple tools at the same time because they are compatible only with specific versions, are extremely painful.

@jasonwilliams
Copy link
Collaborator Author

jasonwilliams commented Nov 3, 2020

This is up to you, if this pull request isn't breaking anything for webpack 4 right now, we can merge it as is, and it will be a huge step forward in supporting webpack 5.

It's good to merge, Webpack 4 will work as normal, all we can do at this point is see what react-dev-utils does.

@sapegin
Copy link
Member

sapegin commented Nov 4, 2020

Cool, then let's merge as soon as it's green. I guess we'll need to ignore coverage on one of the webpack version branches.

https://codewithhugo.com/jest-exclude-coverage/

@jasonwilliams
Copy link
Collaborator Author

@sapegin its a long build, is it hanging?

@sapegin sapegin merged commit 3499c5d into styleguidist:master Nov 5, 2020
@sapegin
Copy link
Member

sapegin commented Nov 5, 2020

It is but it's very very slow now 😢

Merging, thanks for the help!

@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 11.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Support for Webpack v5
3 participants