Update rollup to v3 #26078
Update rollup to v3 #26078
Conversation
|
the bundle size has increased, there may be a problem there and it needs to be investigated. |
There was a problem hiding this comment.
Hey @ymqy thanks for the PRs to bring some overdue updates! This PR is a huge and a difficult to review. I think you've already started separating some smallers parts of these changes into new PRs. I think this'll help a lot.
For example, I see jest related changes in this rollup PR. I think if we can separate them, it'll be a lot easier to review and merge! Thanks for the help!
Thanks for the suggestion, I am preparing to split into smaller separate pr's to make review and merge easier |
|
@kassens Before upgrading rollup, I need to upgrade jest. During the rollup upgrade process, I encountered that jest v26 does not support the 'node:' protocol, so I need to upgrade jest first. I split the jest upgrade into multiple smaller PRs. Now the CI has passed and is ready for review. After merging, I am planning to submit a PR for jest configuration optimization. #26088 |
|
please review #26088 |
|
@ymqy Would be awesome if you could rebase this PR. We still want to land this but updating build tooling needs extra care to not accidentally break releases. |
Of course! I will take care of it shortly |
| @@ -9,4 +9,4 @@ | |||
|
|
|||
| export * from 'react-client/src/ReactFlightClientHostConfigBrowser'; | |||
| export * from 'react-client/src/ReactFlightClientHostConfigStream'; | |||
| export * from 'react-server-dom-webpack/src/ReactFlightClientWebpackBundlerConfig'; | |||
| export * from 'react-server-dom-webpack/src/ReactFlightClientWebpackBundlerConfig.js'; | |||
There was a problem hiding this comment.
I added the '.js' file extension to the import path to fix the ENOENT error. But it is not clear why the '.js' file extension is not added automatically
There was a problem hiding this comment.
These imports seem fragile to me and easy to mess. Why do we now need in some places the explicit .js suffix?
Does it have maybe something to do with this line below?
moduleSideEffects: id => !moduleSideEffects.includes(id),
where the id only matches if the .js exists? (Just a guess, I don't really understand the config 😄 )
| babel({ | ||
| presets: ['@babel/preset-react'], | ||
| sourceMap: true, | ||
| babelHelpers: 'bundled', |
There was a problem hiding this comment.
configure this option explicitly with its default value. https://github.com/rollup/plugins/tree/master/packages/babel#babelhelpers
| @@ -10,7 +10,8 @@ | |||
| import type {ReactNodeList} from 'shared/ReactTypes'; | |||
| import type {BootstrapScriptDescriptor} from 'react-dom-bindings/src/server/ReactDOMServerFormatConfig'; | |||
|
|
|||
| import {Writable, Readable} from 'stream'; | |||
| import type {Writable} from 'stream'; | |||
| import {Readable} from 'stream'; | |||
| @@ -185,7 +186,8 @@ function getRollupOutputOptions( | |||
| format, | |||
| globals, | |||
| freeze: !isProduction, | |||
| interop: false, | |||
| interop: 'default', | |||
There was a problem hiding this comment.
The interop configuration uses the default values to minimize the size increase of the production bundles and pass the ci, and to be honest I'm not very confident about this change.
There was a problem hiding this comment.
From looking at the generated output files, I think this looks mostly right.
There's still a bit of extra code here (you can see the diffs in this comment #26078 (comment) )
Example larger build where the change is visible: https://react-builds.vercel.app/commits/cc7dbd62a503bf502eb911b82a28926cf4d177e9/files/oss-stable/react-debug-tools/cjs/react-debug-tools.development.js?compare=4a4ef2706cfb51db96d48fe20dadcb4fb8e3cb17
There was a problem hiding this comment.
Pretty sure this should be 'esModule' instead of 'default'
scripts/rollup/build.js
Outdated
| module => !importSideEffects[module] | ||
| ); | ||
|
|
||
| const rollupConfig = { | ||
| input: resolvedEntry, | ||
| treeshake: { | ||
| pureExternalModules, | ||
| propertyReadSideEffects: false, |
There was a problem hiding this comment.
I found some code added to the production bundles that reads variables but doesn't use them, so I set propertyReadSideEffects to false to eliminate them
|
@eps1lon There are two other things that confuse me
|
No idea. I'd have to look through how the bundle configs are prepare to understand why some of them are missing now
Maybe a caching issue (have you tested this on a new clone?). Maybe a different environment than CI? |
|
Try again from the beginning. |
|
I've been playing around with a Rollup v3 upgrade on my own machine, and I've run into the same issues:
But, there's another issue I'm seeing locally, and I just checked and it's happening in this PR, too. A noticeable chunk of DOM attribute handling is getting stripped out of the
That seems like a pretty broken change, and I'm not immediately sure what's causing it. updates Will jot down some notes as I play with this.
|
|
Per my above comment and #20186 (comment) , I've got a Rollup v3 upgrade seemingly working locally as part of an attempt to generate sourcemaps. I discussed with Dan, and on Monday I'm going to file 3 separate PRs for fixing bugs in the current Rollup 2.x build config, upgrading to Rollup 3.x, and adding sourcemaps. That will supersede this PR, and I can work on getting those pushed through. @ymqy : if you're around and available to finalize this PR in the next couple days, happy to use this one instead! I just need this wrapped up quickly so I can use it as the basis for the sourcemap generation. |
|
@markerikson Thanks for the advice, I'll try to get as much done as possible. |
|
@ymqy Sure! I think the main thing that's missing in this PR atm is the bugfix for the {
name: 'disable-treeshake',
transform (code, id) {
if (id.endsWith("DOMProperty.js") {
return {
code,
map: null,
moduleSideEffects: 'no-treeshake',
};
}
return null;
},
} |
|
@markerikson I have tried adding the disable-treeshake plugin and found that the react-dom-webpack-server build output still has problems, and have debugged the plugin to verify that the process is correct. Do I need to add |
|
@ymqy : no, the output looked good for me without |
|
@markerikson OK, feel free to @ me if you have any questions.
I'm a bit confused, the problem is still there and the |
|
@ymqy : ended up busy yesterday, but just fetched your branch and taking a look locally now. Will keep you posted on what I figure out! |
|
@ymqy : huh. If I check out your branch and build, I do see the correct output (ie, the Maybe the CI system is still reflecting an old artifact somehow? The one other tweak I think ought to be made to this PR is changing from |
|
@ymqy : try rebasing this against |
|
@ymqy : I filed my own Rollup v3 upgrade PR at #26442 , partly to see if the artifact diffs would come out better. Good news is they did. But, then my PR's CI jobs failed because I was using I spent several more hours trying out several config tweaks to see if I could get Rollup to leave out the import interop helpers and still have I think that means #26442 is ready to be reviewed and merge at this point and would probably supersede this PR, but I specifically wanted to say that I based my changes on what you have here! In particular, the trick of changing the |
|
@markerikson Thank you very much for coming up with a better solution to omit the import interop helpers, and for learning from the big guy's solution. I am also new to contributing to open source and feel honoured to be recognised. At first I just wanted to upgrade Rollup, but I ran into some problems and needed to upgrade jest first (#26088), during which I raised a pr (jestjs/jest#13795) for jest to support the migration from jasmine to jest-circus, and only after this work was done could I start the rollup upgrade, which was additional to the initial work 😂 |
|
@ymqy : hah, yeah, I totally understand that cascading series of "to fix tool X, I first have to update tool Y" problems :) and yeah, I wanted to be clear that I'm not trying to come in and snatch away your chance to contribute here. Even if this PR doesn't get merged specifically, you did a lot of work to figure out what's needed for this upgrade, and I was able to build on top of your work. Really appreciate your efforts here! |
|
@markerikson I hadn't thought of it that way, and I'm glad you're building on my work. Looking forward to seeing you get the sourcemap generation working! |
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR: - Updates Rollup from 2.x to latest 3.x, and updates associated plugins - Updates deprecated / altered config settings in the Rollup plugin pipeline - Fixes some file extension and import issues related to use of ESM in `react-dom-webpack-server` - Removes a now-obsolete `strip-unused-imports` Rollup plugin - <s>Fixes an _existing_ bug with the Rollup 2.x plugin pipeline on `main` that was causing parts of `DOMProperty.js` to get left out of the `react-dom-webpack-server` JS bundles, by adding a new plugin to tell Rollup to treat that file as if it as side effects</s> This PR should be functionally identical to the other existing "Rollup 3 upgrade" PR at #26078 . I'm filing this as a near-duplicate because I'm ready to push this change through ASAP so that I can follow it up with a PR that adds sourcemap support, that PR's artifact diffing seems like it's possibly stuck and I want to compare the build results, and I've got this set up against latest `main`. <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This gets React's build setup updated to the latest Rollup version, which is generally a good practice, but also ensures that any further Rollup config tweaks can be done using the current Rollup docs as a reference. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> - Made builds from the latest `main` - Updated Rollup package versions and cross-compared the changes I needed to make locally to get successful builds vs #26078 - Diffed the output folders between `main` and this PR, and confirmed that the bundle contents are identical (with the exception of version strings and the `react-dom-webpack-server` bundle fix re-adding missing `DOMProperty.js` content)
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary This PR: - Updates Rollup from 2.x to latest 3.x, and updates associated plugins - Updates deprecated / altered config settings in the Rollup plugin pipeline - Fixes some file extension and import issues related to use of ESM in `react-dom-webpack-server` - Removes a now-obsolete `strip-unused-imports` Rollup plugin - <s>Fixes an _existing_ bug with the Rollup 2.x plugin pipeline on `main` that was causing parts of `DOMProperty.js` to get left out of the `react-dom-webpack-server` JS bundles, by adding a new plugin to tell Rollup to treat that file as if it as side effects</s> This PR should be functionally identical to the other existing "Rollup 3 upgrade" PR at #26078 . I'm filing this as a near-duplicate because I'm ready to push this change through ASAP so that I can follow it up with a PR that adds sourcemap support, that PR's artifact diffing seems like it's possibly stuck and I want to compare the build results, and I've got this set up against latest `main`. <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> This gets React's build setup updated to the latest Rollup version, which is generally a good practice, but also ensures that any further Rollup config tweaks can be done using the current Rollup docs as a reference. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> - Made builds from the latest `main` - Updated Rollup package versions and cross-compared the changes I needed to make locally to get successful builds vs #26078 - Diffed the output folders between `main` and this PR, and confirmed that the bundle contents are identical (with the exception of version strings and the `react-dom-webpack-server` bundle fix re-adding missing `DOMProperty.js` content) DiffTrain build for [909c6da](909c6da)
Summary
For the rollup upgrade, I may not be very confident that this can be done, for the configuration changes are only for the ci all through, but there are still some problems need to line up, why some build files deleted does not affect the ci results, ci execution results and local execution results are not consistent, see the following comments
Closes #26078
How did you test this change?
ci green