-
Notifications
You must be signed in to change notification settings - Fork 361
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
update rollup from 2.35.1 (2020-12-14) to at least 3.16.0 (2023-02-17), or ideally latest (4.6.1, 2023-11-30) #1067
Comments
Skimming the
There don't appear to be any obvious breaking changes between There don't appear to be any obvious breaking changes between
There don't appear to be any obvious breaking changes between @rschristian Can you have a quick look at the breaking changes in the If you're happy with those (or those up to a certain point), then I can look at what further changes will be required to bump the |
Eyeballing it, would say 3 is fine, 4 is probably not. This might be a considerable amount of work -- depending on new defaults, might have to do a fair bit of tuning to keep module sizes down. Last I checked, 3 increased our outputs by a non-negligible amount.
Not sure this is the case ("required to implement"), should be doable with a dead-simple plugin, no need for upgrading. Edit: Quick example: function IgnoreListPlugin() {
return {
name: 'ignore-list-plugin',
generateBundle(_opts, bundle) {
const sourcemaps = Object.keys(bundle).filter((file) => /\.map$/.test(file));
for (const file of sourcemaps) {
const map = bundle[file];
map.x_google_ignoreList = [ /* something */ ];
bundle[file].source = JSON.stringify(map);
}
}
}
} |
@rschristian Is there some way to make rollup use this type of custom plugin when bundling with microbundle? |
@daun Only by editing Microbundle's Rollup config directly. |
@rschristian Are you able to give some context on the 'why' of that? Is it the switch from XX to
@rschristian Noted. I figure at the very least I can play around and add some info here that confirms that one way or the other. My current thoughts on approach are to bump to latest Do you have a set of 'canonical repos' you test with I also see the following in
@rschristian Mm, fair enough. I guess I was thinking of it from a 'use the support in the tool' rather than 'roll your own' approach, but yeah, hacking it in via a plugin would probably work as well. Since my mind is in this space though, I figure I'll at least do some initial recon on how big the 'modernise rollup version' work seems like it will end up being. |
I don't love bumping min Node to 18 so quickly.
Not really, I think our test suite here is usually a good way to check that. It could always be better of course, but it'll generally catch most issues.
Long term, sure, but rolling our own will take a fraction of the time to land. It's not exactly complicated functionality to stick an additional key into a sourcemap, after all. |
Looking at the
While it's OOS of this issue, there may also be potential benefits in bumping dependencies such as:
Also:
|
@rschristian nods, makes sense. Though 18 has already passed active support, and has about another 1yr 4mo left on security support; so while it's not required yet, it would make sense to move forward by the time security support ends IMO: That said, at least a cursory skim seems to suggest that
@rschristian Cool, sounds good. Currently builds itself as:
@rschristian nods, valid. I guess I tend to have a default to a perfectionist mindset of 'do it right' (which often leads to scope creep 😅) |
Hrmm.. getting a weird one while trying to test I'll keep looking, but have you ever run into this one before?
It seems that this is coming from the babel transpiled code, where the Details
diff --git a/package.json b/package.json
index 554bebe..a4d7a25 100644
--- a/package.json
+++ b/package.json
@@ -21,7 +21,8 @@
"scripts": {
"build": "npm run -s build:babel && npm run -s build:self",
"build:babel": "babel-node src/cli.js --target=node --format cjs src/{cli,index}.js",
"build:self": "node dist/cli.js --target=node --format cjs src/{cli,index}.js",
+ "//build:self": "node --inspect-brk dist/cli.js --target=node --format cjs src/{cli,index}.js",
"prepare": "npm run -s build",
"prepare:babel": "babel src/*.js -d dist && npm t",
"lint": "eslint src",
@@ -78,12 +79,12 @@
"@babel/preset-env": "^7.12.11",
"@babel/preset-flow": "^7.12.1",
"@babel/preset-react": "^7.12.10",
- "@rollup/plugin-alias": "^3.1.1",
- "@rollup/plugin-babel": "^5.2.2",
- "@rollup/plugin-commonjs": "^17.0.0",
- "@rollup/plugin-json": "^4.1.0",
- "@rollup/plugin-node-resolve": "^11.0.1",
- "@surma/rollup-plugin-off-main-thread": "^2.2.2",
+ "@rollup/plugin-alias": "^5.1.0",
+ "@rollup/plugin-babel": "^6.0.4",
+ "@rollup/plugin-commonjs": "^25.0.7",
+ "@rollup/plugin-json": "^6.0.1",
+ "@rollup/plugin-node-resolve": "^15.2.3",
+ "@surma/rollup-plugin-off-main-thread": "^2.2.3",
"asyncro": "^3.0.0",
"autoprefixer": "^10.1.0",
"babel-plugin-macros": "^3.0.1",
@@ -99,12 +100,13 @@
"lodash.merge": "^4.6.2",
"postcss": "^8.2.1",
"pretty-bytes": "^5.4.1",
- "rollup": "^2.79.1",
+ "resolve-from": "^5.0.0",
+ "rollup": "^3.0.0",
"rollup-plugin-bundle-size": "^1.0.3",
- "rollup-plugin-postcss": "^4.0.0",
+ "rollup-plugin-postcss": "^4.0.2",
"rollup-plugin-terser": "^7.0.2",
- "rollup-plugin-typescript2": "^0.32.0",
- "rollup-plugin-visualizer": "^5.6.0",
+ "rollup-plugin-typescript2": "^0.36.0",
+ "rollup-plugin-visualizer": "^5.10.0",
"sade": "^1.7.4",
"terser": "^5.7.0",
"tiny-glob": "^0.2.8",
Also noticed that after bumping ● fixtures › build visualizer with microbundle
expect(received).toMatchSnapshot()
Snapshot name: `fixtures build visualizer with microbundle 3`
- - Snapshot - 1
+ + Received + 1
- - import a from"camelcase";var o=a("foo-bar");export default o;
+ + import a from"camelcase";var o=a("foo-bar");export{o as default};
//# sourceMappingURL=visualizer.esm.mjs.map Testing with the following command, and bumping just the version of sed -i '' 's/"rollup": ".*"/"rollup": "^2.35.1"/' package.json && \
git checkout package-lock.json && \
rm -rf ./node_modules && \
rm -rf ./dist && \
npm install --ignore-scripts && \
npm test
# Or for a quicker run: npm test -- --testNamePattern 'build visualizer with microbundle' I got the following results:
Presumably it's related to the tree-shaking change? (or something not explicitly mentioned in the CHANGELOG?) So next up is figuring out how to update the relevant fixture snapshots for this so that the tests are less noisy for the next version bumps. Which it seems I can do with a combination of:
⇒ npm test -- --updateSnapshot Failed Tests
After fixing the snapshots to account for the change from Diffdiff --git a/package.json b/package.json
index 554bebe..436dcd1 100644
--- a/package.json
+++ b/package.json
@@ -78,12 +78,12 @@
"@babel/preset-env": "^7.12.11",
"@babel/preset-flow": "^7.12.1",
"@babel/preset-react": "^7.12.10",
- "@rollup/plugin-alias": "^3.1.1",
- "@rollup/plugin-babel": "^5.2.2",
- "@rollup/plugin-commonjs": "^17.0.0",
- "@rollup/plugin-json": "^4.1.0",
- "@rollup/plugin-node-resolve": "^11.0.1",
- "@surma/rollup-plugin-off-main-thread": "^2.2.2",
+ "@rollup/plugin-alias": "^5.1.0",
+ "@rollup/plugin-babel": "^6.0.4",
+ "@rollup/plugin-commonjs": "^25.0.7",
+ "@rollup/plugin-json": "^6.0.1",
+ "@rollup/plugin-node-resolve": "^15.2.3",
+ "@surma/rollup-plugin-off-main-thread": "^2.2.3",
"asyncro": "^3.0.0",
"autoprefixer": "^10.1.0",
"babel-plugin-macros": "^3.0.1",
@@ -101,10 +101,10 @@
"pretty-bytes": "^5.4.1",
"rollup": "^2.79.1",
"rollup-plugin-bundle-size": "^1.0.3",
- "rollup-plugin-postcss": "^4.0.0",
+ "rollup-plugin-postcss": "^4.0.2",
"rollup-plugin-terser": "^7.0.2",
- "rollup-plugin-typescript2": "^0.32.0",
- "rollup-plugin-visualizer": "^5.6.0",
+ "rollup-plugin-typescript2": "^0.36.0",
+ "rollup-plugin-visualizer": "^5.10.0",
"sade": "^1.7.4",
"terser": "^5.7.0",
"tiny-glob": "^0.2.8", Which then leaves us with the following failing tests: Failing Tests
Of which these can have their snapshots updated as they are just some basic CSS order changes/similar: Easy Fixes
But these ones I need to look into more still: Requires further investigation
For those babel ones, there have been a LOT of changes since
Also potentially tangentially relevant:
Might be related to npm why browserslist We should also have a closer look at the CHANGELOG for the following, and potentially try not bumping them fully to the latest versions:
npm why tslib You can see my exploratory WIP branch here: |
I'd hesitate to call that the "right" way -- it's the inbuilt way. Supporting this behavior by plugin doesn't come with any negative repercussions and in fact it'll allow users to continue to use Microbundle on older Node versions. There isn't an inherent need to connect the feature you want with upgrading Rollup in this case.
Usually means something's up with the CJS plugin, yeah. Odd, nothing in
Terser, I believe. That's a better format, generally speaking, as more exports can be added with minimal size increase. |
@rschristian Fair. I guess my brain has already forked from the original task, and so the 'right' that I was referring to here was more about updating the 2-3 year old build tooling to modern versions than it was about the simplest way to implement the original desired change.
@rschristian Also fair. Though I guess at this stage, I'm somewhat interested to see what other benefits can come from modernising the build tooling versions.
@rschristian Yeah, debugging + inspecting it it definitely seems to be a case of the transpiled code not accessing the default properly. Still not sure why that is though.
@rschristian Good to know. It increased the size of most of the test snapshots by a few bytes, but for the two 'more real world' looking ones, the sizes actually decreased by a few bytes. I've been updating the test snapshots as I go (after reviewing that it makes sense to do so) on my WIP branch: |
I'm not sure there's going to be any improvements; Rollup hasn't changed much.
From memory, I think they removed the logic to differentiate in these cases to instead just support the latter; the difference is small and the only ones adversely affected would be modules with a single default export, and only by a few bytes. Might not have been worth it on their end to support. |
Splitting this off as a new issue so it can be tracked more easily. See the original issue for more context:
microbundle
is currently usingrollup
2.35.1
(2020-12-14
):output.sourcemapIgnoreList
(required to implement #1066) support was added in3.16.0
(2023-02-17
):Latest version (at time of writing) is
4.6.1
(2023-11-30
):The text was updated successfully, but these errors were encountered: