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.

Sign up for GitHub

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

Jump to bottom

Update to Rollup 2 #889

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update to Rollup 2 #889

wants to merge 1 commit into from
+161 −126

Conversation

ludofischer
Copy link
Contributor

@ludofischer ludofischer commented Sep 28, 2020 •
edited by agilgur5

  • Update Rollup
  • Update plugin-commonjs, plugin-json, plugin-replace, plugin-terser, plugin-postcss
    to versions officially supporting Rollup 2

(maintainer edit to add tags as this relates to a lot of issues):

Tags

Resolves some issues:

Closes out some related / now duplicative PRs:

@vercel

This comment has been minimized.

Sign in to view
@ludofischer
Copy link
Contributor Author

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 sourcemap option anymore, instead it infers it from the rollup options (see https://github.com/TrySound/rollup-plugin-terser/releases/tag/v6.0.0)

@agilgur5 agilgur5 added topic: Rollup 2 Related to Rollup 2 upgrade version: minor Increment the minor version when merged labels Sep 28, 2020
@agilgur5 agilgur5 changed the title Update to Rollup 2. Update to Rollup 2 Sep 28, 2020
@agilgur5 agilgur5 added the topic: Node 10+ requires Node 10+ label Sep 28, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 28, 2020 •
edited

I believed this would be harder, am I missing something?

Rollup has not been upgraded for lack of effort, it's because it is a very breaking change, likely affecting a good portion of tsdx.config.js users. See #821 (comment).
As mentioned there, previously it was also not well supported, and I believe there are still issues with TS 3.8+ as I mentioned in that comment and where it links to: #679 (comment)
It also drops Node 8 support, as does plugin-terser. TSDX v0.14.0 was just released a week ago that now requires Node 10+ -- Node 8 was still supported until a week ago.

I also wrote in the release notes of v0.14.0:

I also wanted to push Rollup 2, TS 4.0, Prettier 2, and ESLint upgrades into this, but the breaking changes in the changelog started getting too big, so I decided to wait a bit to split those changes across more releases to not throw too much breakage at users at once.

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 tsdx.config.js users. But as TS 4, Prettier 2, and ESLint were left out of v0.14.0 intentionally, I was thinking of shipping those as v0.15.0 and pushing what's currently slated as that until v0.16.0. Those could be interchanged though.

This was referenced Sep 28, 2020
Closed
Closed
Closed
This was linked to issues Sep 28, 2020
Upgrade ts-jest and rollup-plugin-terser to fix vulnerabilities #803
Closed
Update rollup-plugin-terser to fix vulnerability #797
Closed
@ludofischer
Copy link
Contributor Author

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.

@agilgur5 agilgur5 linked an issue Sep 28, 2020 that may be closed by this pull request
Rollup 2 support #821
Closed
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 28, 2020 •
edited

@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.
Some maintainers can widen out APIs too quickly and also break APIs unknowingly frequently; the former makes the latter an easier mistake to make.

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 async-to-promises and the recent switch to regenerator in #795).
tsdx.config.js opens up basically the whole Rollup API as a TSDX API that can be broken. That's why I discourage it and say to only use if necessary as an escape hatch, per the warning I added in #400 (when I was still a contributor). It's very difficult to promise that we'll know if anything breaks that, and nearly any plugin change or minor change could be considered breaking, so that warning is also a warning that unforeseen breakage is to be expected with tsdx.config.js. This specific PR's breakage is quite foreseen though, so I think good faith should be made to plan it as a breaking change and notify users.
Being an all-in-one tool also further increases this scope. A breaking change to any one of tsdx build, tsdx test, tsdx lint, tsdx create, etc also mean that I have to release a breaking change for the entirety of TSDX since they're all currently combined as one big API of TSDX.

#635 is one of my approaches to improve the tsdx.config.js problem (make maintainable and testable plugins) and #634 is an approach to start splitting out some of the API. I'd like to split out each command into a separate package as well, that way I can work on improving, say, @tsdx/test independently and release a breaking change there without affecting tsdx test until that is upgraded. Can then batch breaking changes together more easily and also allow users to opt-out or opt-in to different pieces of TSDX without requiring the whole thing. It can also allow for things like @tsdx/jest vs. @tsdx/mocha, etc.

agilgur5
agilgur5 requested changes Sep 28, 2020
View reviewed changes
Copy link
Collaborator

@agilgur5 agilgur5 left a 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

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
src/createRollupConfig.ts Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
@agilgur5 agilgur5 added the scope: dependencies Pull requests that update a dependency file label Sep 28, 2020
This was referenced Sep 28, 2020
Merged
Open
@ludofischer
Update to Rollup 2.
f9b6340 
 - 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.
@agilgur5 agilgur5 mentioned this pull request Sep 30, 2020
Closed
agilgur5
agilgur5 approved these changes Sep 30, 2020
View reviewed changes
Copy link
Collaborator

@agilgur5 agilgur5 left a 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.

@ludofischer
Copy link
Contributor Author

Great! I’ve been happy to help.

@agilgur5 agilgur5 mentioned this pull request Oct 7, 2020
Merged
@agilgur5 agilgur5 mentioned this pull request Feb 18, 2021
Closed
@bartsidee bartsidee mentioned this pull request Feb 18, 2021
Closed
@agilgur5 agilgur5 mentioned this pull request Mar 16, 2021
Closed
@agilgur5 agilgur5 mentioned this pull request Jul 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agilgur5 agilgur5 agilgur5 approved these changes

Assignees
No one assigned
Labels
scope: dependencies Pull requests that update a dependency file topic: Node 10+ requires Node 10+ topic: Rollup 2 Related to Rollup 2 upgrade version: minor Increment the minor version when merged
Projects
None yet
Milestone
v0.15.0 - Rollup 2 + multi-entry + `p...
2 participants
@ludofischer @agilgur5