update ReactFlightWebpackPlugin to be compatiable with webpack v5 #22739
Conversation
|
Hi @michenly! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
1 similar comment
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -268,7 +330,8 @@ export default class ReactFlightWebpackPlugin { | |||
| (err2: null | Error, deps: Array<ModuleDependency>) => { | |||
| if (err2) return cb(err2); | |||
| const clientRefDeps = deps.map(dep => { | |||
| const request = join(resolvedDirectory, dep.request); | |||
| // use userRequest instead of request. request always end with undefined which is wrong | |||
| const request = join(resolvedDirectory, dep.userRequest); | |||
There was a problem hiding this comment.
the only part I slightly question
| compilation.hooks.processAssets.tap( | ||
| { | ||
| name: PLUGIN_NAME, | ||
| stage: Compilation.PROCESS_ASSETS_STAGE_REPORT, |
There was a problem hiding this comment.
Should this be PROCESS_ASSETS_STAGE_REPORT or PROCESS_ASSETS_STAGE_DERIVED?
There was a problem hiding this comment.
PROCESS_ASSETS_STAGE_REPORT — creating assets for the reporting purposes.
PROCESS_ASSETS_STAGE_DERIVED — derive new assets from the existing assets.
They both sound possible from the description alone.
However, I went with PROCESS_ASSETS_STAGE_REPORT because the type def of PROCESS_ASSETS_STAGE_DERIVED had mentioned NOT treating existing assets as complete which is not the scenario here.
| }); | ||
| }; | ||
|
|
||
| normalModuleFactory.hooks.parser |
There was a problem hiding this comment.
Why is this better than tapping into buildModule?
There was a problem hiding this comment.
buildModule in webpack 5 no longer let you modify dependencies. (there is actually a line right after the hook that wipes it out, see details here).
parser is the recommended way going forward
|
Looks good to me, thanks. |
|
@gaearon is it possible for me to publish a new version of |
Summary: This sync includes the following changes: - **[f2a59df48](facebook/react@f2a59df48 )**: Remove unstableAvoidThisFallback from OSS ([#22884](facebook/react#22884)) //<salazarm>// - **[24dd07bd2](facebook/react@24dd07bd2 )**: Add custom element property support behind a flag ([#22184](facebook/react#22184)) //<Joey Arhar>// - **[72e48b8e1](facebook/react@72e48b8e1 )**: Fix: Don't skip writing updated package.json //<Andrew Clark>// - **[e39b2c899](facebook/react@e39b2c899 )**: Fix peer deps for use-sync-external-store //<Andrew Clark>// - **[ec78b135f](facebook/react@ec78b135f )**: Don't override use-sync-external-store peerDeps ([#22882](facebook/react#22882)) //<Andrew Clark>// - **[5041c37d2](facebook/react@5041c37d2 )**: Remove hydrate option from createRoot ([#22878](facebook/react#22878)) //<salazarm>// - **[3f9480f0f](facebook/react@3f9480f0f )**: enable continuous replay flag ([#22863](facebook/react#22863)) //<salazarm>// - **[4729ff6d1](facebook/react@4729ff6d1 )**: Implement identifierPrefix option for useId ([#22855](facebook/react#22855)) //<Andrew Clark>// - **[ed00d2c3d](facebook/react@ed00d2c3d )**: Remove unused flag ([#22854](facebook/react#22854)) //<Dan Abramov>// - **[0cc724c77](facebook/react@0cc724c77 )**: update ReactFlightWebpackPlugin to be compatiable with webpack v5 ([#22739](facebook/react#22739)) //<Michelle Chen>// - **[4e6eec69b](facebook/react@4e6eec69b )**: fix: document can be `null`, not just `undefined` ([#22695](facebook/react#22695)) //<Simen Bekkhus>// Changelog: [General][Changed] - React Native sync for revisions c1220eb...a049aa0 jest_e2e[run_all_tests] Reviewed By: rickhanlonii Differential Revision: D33062386 fbshipit-source-id: 37e497947efad5696c251096da8a92ccdc6dcea7
|
I just had a chance to review this properly. Thanks for the detailed work here and preserving the exact details! |
Summary
Current React Server Component uses webpack 4 when webpack 5 had been out for more than 1 year.
This PR updated the current plugin
packages/react-server-dom-webpack/src/ReactFlightWebpackPlugin.jsto be compatible with the latest webpack 5How did you test this change?
The original development was done in reactjs/server-components-demo#52 and written in typescript.
I tested it by using the plugin in
server-components-demoprojectnpm run startreact-client-manifest.jsonbuild output in webpack 4 & webpack 5 (the content are exactly the same with the exception of vendor files are named differently in webpack 5)