-
Notifications
You must be signed in to change notification settings - Fork 508
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 to Rollup 2 #889
base: master
Are you sure you want to change the base?
Update to Rollup 2 #889
Conversation
This comment has been minimized.
This comment has been minimized.
Looks like the MacOS failure is anetwork problem on the test machine. I believed this would be harder, am I missing something? the only change to the code itself is that the terser plugin does not accept a |
Rollup has not been upgraded for lack of effort, it's because it is a very breaking change, likely affecting a good portion of I also wrote in the release notes of v0.14.0:
I'm not planning on releasing Rollup 2 at least for a few weeks to spread out aforementioned breakage, and I was planning to batch that in with v0.15.0's changes, which also are only breaking to a subset of |
Yes, as I was making the changes I realized maintaining a compiler tool like this must be horribly complicated as it’s hard to check if and how you’re breaking other projects. It’s even harder as you’re not really in control of what upstream dependencies decide to release. |
@ludovicofischer indeed. I appreciate the acknowledgement and understanding. Breaking changes really require a lot of thought and planning of maintainers and lot of factors in TSDX don't make that easier. That's one of the reasons why the Unix methodology of small modules doing one thing well is a good one to follow -- the smaller the API surface, the less potential for breakage there is, and the more known what that breakage looks like. The scoping problem becomes larger with the introduction of any new API, which is why those decisions should really be made quite carefully. Being a compiler tool as you say increases the scope and potential impact of breakage to be quite large; it can literally break the correctness of your built code (see also the problems with #635 is one of my approaches to improve the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @ludovicofischer !
I've made several comments here. Highlights are:
- Per Upgrade from rollup-plugin-babel to @rollup/plugin-babel #789 (review), writing a rationale and linking to the changelog for each change and describing them in the PR and commit makes it much, much, easier to review. I've added comments here that describe, from my perspective, why it seems like each of these changes were made.
- The devDep,
rollup-plugin-postcss
should be upgraded as well. There's an integration test for it yarn.lock
changes seem to be too broad
- Update Rollup to 2.28.2. Fixes jaredpalmer#821, closes jaredpalmer#545 - Update @rollup/plugin-commonjs. Upgrading this required Rollup 2 without any note in the changelog. Closes jaredpalmer#727 - Update @rollup/plugin-json to 4.1.0. v4.0.3 is the first to add Rollup 2 in the peerDep range. Older versions are forward-compatible but will produce a peerDep warning - Update @rollup/plugin-replace to 2.3.3. v2.3.2 is the first version to add Rollup 2 in the peerDep range. - Update rollup-plugin-terser to v7. v6 requires rollup 2 and Node 10+. v7 introduces Terser 5, requires Node >= 10 and supports some new JS syntax. fixes jaredpalmer#803, #fixes 797, closes jaredpalmer#731 - Update rollup-plugin-postcss to 3.1. Closes jaredpalmer#693. - Remove sourcemap option from terser rollup plugin config, as of rollup-plugin-terser v6.0, it’s inferred automatically from Rollup’s output.source config.
971b2b9
to
f9b6340
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the iteration @ludovicofischer ! Seems like all the issues I pointed out are now resolved.
Tests all pass and no stray warnings or errors in the logs.
I've also deleted ~56 unnecessary Greenkeeper branches that were associated with the related PRs that this consolidates and updates.
This is ready to go, but per the version label, please to any other folks with write access DO NOT MERGE until the v0.15.0 release is being prepared, as, to repeat, this is quite BREAKING.
Great! I’ve been happy to help. |
to versions officially supporting Rollup 2
(maintainer edit to add tags as this relates to a lot of issues):
Tags
Resolves some issues:
rollup-plugin-terser
: fixes Upgrade ts-jest and rollup-plugin-terser to fix vulnerabilities #803, fixes Update rollup-plugin-terser to fix vulnerability #797Closes out some related / now duplicative PRs: