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

fix(alias,auto-install,babel,beep,buble,commonjs,data-uri,dsv,dynamic-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml): ensure rollup 4 compatibility #1595

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Sep 27, 2023

Rollup Plugin Name: alias,auto-install,babel,beep,buble,commonjs,data-uri,dsv,dynamic-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (well, adapted...)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:

Description

Rollup 4 is nearly ready for release. As a matter of fact I am hoping for a release by Thursday next week (i.e. at ViteConf). However, this PR would need to be merged first as otherwise, plugins are not usable for Rollup 4 users.

There should be no breaking changes in this release.

This is a list of all changes:

  • Updated the peerDependency versions of Rollup to include ^4.0.0
  • Replaced assertions with attributes in test snapshots of resolve options as that was a major change. Apparently, it does not affect any plugins, though, as they ignore that attribute.
  • commonjs
    • As parse errors no longer contain a loc attribute but only a pos attribute, the logic was changed to forward this attribute to Rollup on errors instead of loc. This is non-breaking as earlier Rollup versions already had pos as well.
    • Added explicit skipSelf: false to this.resolve calls where unspecified as the default value has changed. Again, this is non-breaking as it just explicitly states the previous default.
    • Explicitly skip transforming the value after parsing the key of a shorthand property. Before, they were only deep equal while now, they are the same reference. This trips up is-reference, but I think this is the best way to resolve this. Again, this should have no effect on earlier Rollup versions.
  • Updated some snapshots to account for minor changes, e.g. Errors no longer have an own toString method.
  • node-resolve
    • added explicit skipSelf: false to this.resolve calls where unspecified as the default value has changed.
  • typescript
    • replaced RollupWarning type with a custom RollupLog type containing only the properties we need. Once we drop rollup@2 support, this can be replaced by the real RollupLog type again. This avoids a breaking change. I added a corresponding TODO comment.
    • Added some type casts to support querying inputOptions.preserveModules on older Rollup versions, even though this property has been removed now. I added a TODO comment to remove the corresponding check once we dropped rollup@3 support.

@shellscape I hope I got the commit message right this time 😉

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

github-actions[bot]

This comment was marked as resolved.

@lukastaegert lukastaegert changed the title Make all plugins ready for Rollup 4 fix(alias,auto-install,babel,beep,buble,commonjs,data-uri,dsv,dynamic-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml): ensure rollup 4 compatibility Sep 27, 2023
Copy link
Member

@tada5hi tada5hi left a comment

Choose a reason for hiding this comment

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

The failing windows ci should not be related to the changes

@lukastaegert
Copy link
Member Author

The failing windows ci should not be related to the changes

That would be a relief, but why is it failing? Is it broken on master?

@tada5hi
Copy link
Member

tada5hi commented Oct 3, 2023

@lukastaegert think another consideration we could make is to set the setting --workspace-concurrency to 1. What do you think ?

@shellscape
Copy link
Collaborator

For the Windows build it seems like a good idea. We should also report this to pnpm

@lukastaegert lukastaegert force-pushed the fix/rollup-4-compatibility branch 4 times, most recently from 8fa06b5 to 7c56ce3 Compare October 3, 2023 19:31
@lukastaegert
Copy link
Member Author

Forcing ppm to 8.8.0 did not solve the issue. Adding --workspace-concurrency 1 to the Windows test run works, though, as you can see.

@lukastaegert lukastaegert force-pushed the fix/rollup-4-compatibility branch 2 times, most recently from b992fba to ff83e1f Compare October 4, 2023 04:32
@lukastaegert
Copy link
Member Author

I slightly reworked the changes in the TypeScript plugin so that there is no change in the productive code at all and no potential for breakage. I also added two TODO comments to change the code once we dropped support for rollup@2 and rollup@3 respectively.

@shellscape Do you see any chance we can merge this tomorrow? This would allow me to release Rollup 4 in time for ViteConf. The productive changes are now very minimal and non-breaking, otherwise it mostly concerns test code and snapshots, see the list in the PR description.

@shellscape
Copy link
Collaborator

I'm going to be out of pocket for the rest of the week with travel unfortunately. BUT I've been following along and I see no reason not to push it forward. You've got my backing.

@lukastaegert
Copy link
Member Author

Perfect, thanks so much!

@sapphi-red
Copy link
Contributor

While trying out Rollup v4 on the Vite repo (I didn't use the plugins built on this PR's branch), I found that the rebuild doesn't happen when I change the file while running rollup --watch. I guess this change (rollup/rollup#5150) now requires the typescript plugin to call this.addWatchFile.

@lukastaegert
Copy link
Member Author

@sapphi-red Great catch! I added this and a test, should be working now!

…-import-vars,eslint,esm-shim,graphql,html,image,inject,json,legacy,multi-entry,node-resolve,pluginutils,replace,run,strip,sucrase,swc,terser,typescript,url,virtual,wasm,yaml): ensure rollup 4 compatibility
@sapphi-red
Copy link
Contributor

Thanks! I have tried it locally on the Vite repo and confirmed that it works. 👍

@lukastaegert lukastaegert merged commit 841a039 into master Oct 5, 2023
5 checks passed
@lukastaegert lukastaegert deleted the fix/rollup-4-compatibility branch October 5, 2023 11:54
@lukastaegert
Copy link
Member Author

Ok, it seems there was a hiccup with GitHub during release. Essentially, all packages before sucrase were successful but all others were not published. Worse, the changelog commit was also not generated. I am wondering how to best remedy this, but I guess I should leave it as it is. A subsequent publish run would fail as well as it would try to publish over the already successfully published versions.
In order to have the Rollup release unblocked, I would manually release patch versions of the remaining packages without updating the repository. Then the error is at least symmetric across all packages, i.e. every package is one patch version behind, and we will need to see how to clean up the repo later and update the changelog.

@lukastaegert
Copy link
Member Author

Ok, sucrase seems to be in a weird state. npm show @rollup/plugin-sucrase versions says that 5.0.1 is the latest version, but publishing 5.0.2 fails because it thinks that the version already exists. All other plugins are updated but as I said, the changelog is not updated. I will leave it in that state.

@tada5hi
Copy link
Member

tada5hi commented Oct 5, 2023

Ok, it seems there was a hiccup with GitHub during release. Essentially, all packages before sucrase were successful but all others were not published. Worse, the changelog commit was also not generated. I am wondering how to best remedy this, but I guess I should leave it as it is. A subsequent publish run would fail as well as it would try to publish over the already successfully published versions.

In order to have the Rollup release unblocked, I would manually release patch versions of the remaining packages without updating the repository. Then the error is at least symmetric across all packages, i.e. every package is one patch version behind, and we will need to see how to clean up the repo later and update the changelog.

Another possibility would be to merge pull request #1585, since this also targets all repos. However, this would not solve the problem that for some packages no previous version exists.

@lukastaegert
Copy link
Member Author

Exactly. Maybe the solution would be to just manually add a commit that raises all patch versions by 1 to master, and THEN merge the other PR. I would assume that this would then generated the changelog containing the entries from both PRs. But I would prefer to wait until @shellscape is back to have a look.

@shellscape
Copy link
Collaborator

shellscape commented Oct 8, 2023

I haven't been able to closely follow along, so not entire sure how we got into the situation. I'll take a look at this today to sort it out.

OK after looking at the state of things, this is a real mess. Each plugin is going to have to be checked and have things manually updated. That's going to be a lift. Moving forward, we should wait until we have someone around who is intimately familiar with the release process before pushing a big one out like this. (And I would prefer that to be more than just myself)

Or, do smaller groups of changes to contain.

@shellscape
Copy link
Collaborator

@lukastaegert did you manually publish these? The workflow log doesn't show any publishing activity other than a failure on alias, which should have subsequently stopped anything else from being published.

@shellscape
Copy link
Collaborator

OK all of the changelogs and package.json version properties are updated, and I've tagged the repo with the versions as well. We should be in good shape. I'll be looking into process hardening in the near future here.

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.

None yet

4 participants