-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
feat!: rollup v4 #14508
feat!: rollup v4 #14508
Conversation
…tions to `this.parse`
ef728b9
to
176f6d5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Most of the fails are type incompatibility errors. This analog fail might be a bug in rollup. |
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.
Changes look great so far. Nice to see there's not too many change needed on our end.
Back to green 🫡 |
All Rollup plugins (except sucrase, there was a weird hiccup in the pipeline) have been updated (thought the changelog does not reflect it...). |
Ah sorry about the additional change. I think GitHub does an internal merge by default when testing. Seem to see it happening more recently. |
No problem 😃 Hmm, I see. Yeah, it seems to be working like that 🧐 |
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
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.
LGTM. If we want to stay on track with the release schedule, I think we should merge this as soon as possible.
To make the ecosystem-ci slightly greener so we don't catch type issues, maybe we can override rollup to v4 too.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
Isn't parse exposed from |
Yeah we've updated to use that now. It was discussed at #14508 (comment) |
Description
This PR upgrades Rollup to v4.
Remaining TODOs:
this.addWatchFile
is needed in other placespackage.json
refs rollup/rollup#5140
close #13342
Additional context
this.parse
Rollup now uses SWC with a custom tree converter (converts SWC AST to ESTree AST) to parse JS.
This PR gets
this.parse
from rollup by creating a dummy bundle and uses that in dev. I'm not sure if this is legal 😅This way we can be 100% compatible with rollup.
The other way is to use acorn in dev. But it seems even if we added both
acorn-import-attributes
andacorn-import-assertions
, acorn cannot parseasserts
. So we have to create our own acorn plugin or remove support for eitherasserts
if we take this way.skipSelf: true
is now defaultMaybe #13852 should be given more priority now.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).