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.

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

chore: update all rollup dependencies to run CI with #2114

Merged
merged 9 commits into from
Mar 16, 2023

Conversation

Westbrook
Copy link
Member

What I did

  1. Take latest rollup dependencies across the board.
  2. Open a draft PR to test the versions on CI.
  3. If it works, we can have the discussion about mixed version resolutions or hard breaking release or whatever.
  4. If not...something else 😄

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2023

🦋 Changeset detected

Latest commit: b15c21b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@web/dev-server-rollup Minor
@web/dev-server-storybook Minor
@web/dev-server Minor
@web/rollup-plugin-html Major
@web/rollup-plugin-import-meta-assets Major
@web/rollup-plugin-polyfills-loader Major
rollup-plugin-workbox Major
@web/test-runner-mocha Minor
@web/test-runner Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch from 74d894c to 0a06b42 Compare February 22, 2023 13:36
@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch 3 times, most recently from d1d61e9 to 6cdad36 Compare February 22, 2023 15:29
@Westbrook Westbrook changed the base branch from master to next February 24, 2023 02:20
@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch 2 times, most recently from 3a75536 to 0dc7612 Compare February 24, 2023 02:47
@Westbrook
Copy link
Member Author

A follow up question for this (if we can get tests passing) will end up being "do we make a Rollup@3 only release?"

@43081j
Copy link
Contributor

43081j commented Feb 24, 2023

@Westbrook it looks like rollup plugins have changed structure...

before, a plugin had {resolveId: Function}

now it has {resolveId: {handler: Function, order: 'pre'|'post'}}

r3.patch

@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch 2 times, most recently from 8214fec to 482bb44 Compare February 24, 2023 19:32
Base automatically changed from next to master March 11, 2023 21:06
@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch from 482bb44 to 69778ea Compare March 14, 2023 23:55
@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch from 69778ea to 479b74b Compare March 15, 2023 01:17
@Westbrook Westbrook force-pushed the westbrook/try-rollup-3 branch from 03ba7b7 to 6974dd5 Compare March 15, 2023 02:33
@Westbrook
Copy link
Member Author

I've very naively taken all of the changes that come with this update and applied them to the tests. They feel OK-ish for the most part, but I don't 100% know what should be in this area.

@koddsson
Copy link
Contributor

koddsson commented Mar 15, 2023

@Westbrook This is great work! Is there something I can do to help with this PR?

Maybe we can do a pre-release and start testing in various environment. I'd love to take this for a spin on projects at ING to see if there's something broken that we can find.

@Westbrook
Copy link
Member Author

@koddsson A pre-release is probably a good idea! I've mainly just assumed that the process worked and accepted all of the new test rules. They're stable... which is something.

If you were well versed in the sorts of transforms we're doing with CJS and ESM, extra eyes on the updated tests outputs would be a great first step. @43081j has been working a lot in this area recently, and I hope he has some time to check these changes out as well.

The other area that I can think of right now would be what sort of version change this is. Technically, I don't think our APIs change, but I guess we do remove support for Rollup@2, which would be a breaking change. Would everything need to break? Just @web/dev-server-rollup and similar? Something else?

@koddsson
Copy link
Contributor

@koddsson A pre-release is probably a good idea! I've mainly just assumed that the process worked and accepted all of the new test rules. They're stable... which is something.

Yeah, I might be overly cautious here 😄 If the tests are green, that might just be fine. Then if there are issues we can make bugfix releases.

If you were well versed in the sorts of transforms we're doing with CJS and ESM, extra eyes on the updated tests outputs would be a great first step. @43081j has been working a lot in this area recently, and I hope he has some time to check these changes out as well.

I can take a look and tell you if I spot something out of the ordinary 😄

The other area that I can think of right now would be what sort of version change this is. Technically, I don't think our APIs change, but I guess we do remove support for Rollup@2, which would be a breaking change. Would everything need to break? Just @web/dev-server-rollup and similar? Something else?

I think a breaking change for @web/dev-server-rollup makes sense. Looking at the changes, I think we should probably make a breaking changes for anything that upgrades from Rollup 2 to Rollup 3 so that we don't accidentally break people's build with this change. Packages with no Rollup updates like test-runner-mocha can just go out in a patch release.

@Westbrook Westbrook changed the base branch from master to next March 16, 2023 16:59
@Westbrook Westbrook marked this pull request as ready for review March 16, 2023 17:34
@Westbrook
Copy link
Member Author

Going to prerelease this now! 🤞🏼

@Westbrook Westbrook merged commit b150e0d into next Mar 16, 2023
@Westbrook Westbrook deleted the westbrook/try-rollup-3 branch March 16, 2023 17:54
@Westbrook
Copy link
Member Author

🦋  success packages published successfully:
🦋  @web/dev-server@0.0.0-canary-20230316175616
🦋  @web/dev-server-esbuild@0.0.0-canary-20230316175616
🦋  @web/dev-server-rollup@0.0.0-canary-20230316175616
🦋  @web/dev-server-storybook@0.0.0-canary-20230316175616
🦋  @web/rollup-plugin-html@0.0.0-canary-20230316175616
🦋  @web/rollup-plugin-import-meta-assets@0.0.0-canary-20230316175616
🦋  @web/rollup-plugin-polyfills-loader@0.0.0-canary-20230316175616
🦋  rollup-plugin-workbox@0.0.0-canary-20230316175616
🦋  @web/test-runner@0.0.0-canary-20230316175616
🦋  @web/test-runner-mocha@0.0.0-canary-20230316175616

@koddsson
Copy link
Contributor

I've updated @web/dev-server, @web/test-runner and @web/dev-server-storybook in ~20 internal repositories and everything is looking great ✨

@@ -33,7 +34,7 @@ describe('commonjs', () => {
});

it('can handle require default', () => {
expect(requiredDefault).to.equal('foo');
expect(requiredDefault.default).to.equal('foo');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@43081j this feels like an error to have converted, in that this same wrapping of default is causing me some trouble with the canaries...thoughts?

@@ -695,6 +695,7 @@ describe('rollup-plugin-html', () => {
}

const outputHtml = getAsset(output, 'index.html').source;
console.log(outputHtml);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a console.log was left in here ☝🏻

@@ -24,6 +24,7 @@ describe('commonjs', () => {
});

it('can handle compiled es modules with named exports', () => {
console.log(compiledEsm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this console.log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants