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

feat(core): upgrade to webpack-dev-server@4 #5420

Merged
merged 19 commits into from Oct 28, 2021
Merged

feat(core): upgrade to webpack-dev-server@4 #5420

merged 19 commits into from Oct 28, 2021

Conversation

AviVahl
Copy link
Contributor

@AviVahl AviVahl commented Aug 25, 2021

Motivation

  • current webpack-dev-server@3 has deps with known security issues.
  • v4 has been released as stable
  • use react-dev-utils@next for temporary Webpack 5 support (instead of our copied files)
  • Server /static folder with the official static option instead of an express middleware
  • Cleanup old/unused configs

Used: https://github.com/webpack/webpack-dev-server/blob/master/migration-v4.md

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

  • yarn start works with a good DX, hot reload and show new error overlay
  • CI pass tests

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 25, 2021
@AviVahl
Copy link
Contributor Author

AviVahl commented Aug 25, 2021

TBH, I'm not sure whether the tests cover the start flow or hot reloading. Given a custom client/overlay is used, further validation of this feels needed.

The logging options were removed from DevServer itself, as it now uses webpack's logging mechanism.
It might be needed to adjust the used webpack config logging options to replicate the existing behavior.

@netlify
Copy link

netlify bot commented Aug 25, 2021

✔️ [V2]

🔨 Explore the source changes: 356daa3

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/617ac69b8f96040008d7e268

😎 Browse the preview: https://deploy-preview-5420--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Aug 25, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 66
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5420--docusaurus-2.netlify.app/

@slorber
Copy link
Collaborator

slorber commented Aug 26, 2021

Thanks!

I am able to start the dev server successfully (yarn install && yarn start:website)

However there are a few issues:

  • Logging is much more verbose than before, how can we revert this?
  • Hot reload does not actually work (both for React and md files). I see Webpack recompiling but no browser hot reload, and a refresh is required to see the changes)

Can you add back the useless ...{} spread? We'll remove this once we get it working. In the meantime it helps to review this PR otherwise I see a larger diff than there are actual changes.

@slorber
Copy link
Collaborator

slorber commented Aug 26, 2021

@slorber
Copy link
Collaborator

slorber commented Aug 26, 2021

Some random notes:

  • I'm not sure CRA still use the custom hot dev client that we initially copied to Docusaurus. Doc says it's an alternate dev client and only works for v3. The PR above seems to not use it, and does not use client: false like us. (https://github.com/facebook/create-react-app/tree/main/packages/react-dev-utils#webpackhotdevclient)

  • Not using client: false leads to hot reload working. I'd say we should just move on and let Webpack do its job without re-inventing the wheel. It's not clear to me why we use a custom hot dev client in the first place 😅 will try to find a reason

  • Using infrastructureLogging: {level: 'error'} (parent config, not dev serverà) can silence some logs:

image

However, I'm not able to get rid of the following logs that we didn't have before:

image

As not all Docusaurus users are skilled frontend devs, I'd prefer not to show anything too complicated in the console ;)

@alexander-akait any idea?

@AviVahl
Copy link
Contributor Author

AviVahl commented Aug 26, 2021

Thanks for looking into this! :)

The ..{ causes excess configuration fields to not error. Since the types are much more accurate, I suggest removing it.

If you add stats: 'errors-warnings' to the webpack config, it may behave closer to what you expect I think...

@alexander-akait
Copy link

@slorber for reduce stats output you can use devServer.devMiddleware.stats, anyway what do you think extra here? We can reduce output

@slorber
Copy link
Collaborator

slorber commented Sep 1, 2021

Thanks

To sum-up:

I think we'll follow what CRA do and use the new Webpack overlay features

I'm on holiday in 2 days so will wait for my return to merge/release this.

@alexander-akait
Copy link

Also will be great any feedback/help with out current overlay, improving is not hard, just need some help here

@AviVahl
Copy link
Contributor Author

AviVahl commented Sep 27, 2021

I've merged main and bumped webpack-dev-server to v4.3.0. All tests pass locally.

@slorber
Copy link
Collaborator

slorber commented Oct 7, 2021

I'll make sure this PR is merged asap, just want to do a last stable release before merging it so that we have time to see dev issues by using it ourselves

@alexander-akait
Copy link

In near future I will implement eTag and Modified-Since headers for caching, so browser will be faster with getting output, especial in incremental build with caches

@slorber
Copy link
Collaborator

slorber commented Oct 15, 2021

In near future I will implement eTag and Modified-Since headers for caching, so browser will be faster with getting output, especial in incremental build with caches

Great, never thought about this in dev but it makes sense 👍 Let me know when we should upgrade and test this.


@AviVahl we just released beta.7.

If there's no major bug reported in the upcoming day I'll complete and merge this PR next week.

@slorber
Copy link
Collaborator

slorber commented Oct 21, 2021

beta.8 bugfix released, hope to merge this PR next week then 😅

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 27, 2021
@slorber slorber changed the title chore(deps): upgrade to webpack-dev-server@4 feat(core): upgrade to webpack-dev-server@4 Oct 27, 2021
@slorber slorber mentioned this pull request Oct 28, 2021
@@ -98,7 +97,7 @@
"postcss": "^8.3.7",
"postcss-loader": "^6.1.1",
"prompts": "^2.4.1",
"react-dev-utils": "^11.0.1",
"react-dev-utils": "12.0.0-next.47",
Copy link
Collaborator

Choose a reason for hiding this comment

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

temporary pinning this dep, react-dev-utils package seems stable enough for usage in Docusaurus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants