-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow parsing the with
keyword for import attributes
#21
Conversation
}, | ||
"assertions": [ | ||
{ | ||
"type": "ImportAttribute", |
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.
Just a though. We could preserve the with
/assert
keyword used in the ImportAttribute node, tooling could warn users to migrate.
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 @nicolo-ribaudo
"value": "./foo.json", | ||
"raw": "\"./foo.json\"" | ||
}, | ||
"assertions": [ |
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.
Looking at (the consensus?) https://github.com/estree/estree/blob/master/experimental/import-attributes.md, shouldn't this be changed to attributes
? Any interest in pursuing this? From Rollup side, I would much prefer to go with the spec, even if it means we need to do some code changes. At that point, one could also see that in the absence of attributes, attributes
is an empty array (in previous versions, assertions
was undefined
, which deviates both from ESTree and the behavior of other acorn nodes, cf #15).
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.
Sorry for being a little late on this one.
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.
Yes :) I think we should publish that as a new acorn-import-attributes
package that properly supports this feature.
My goal with this PR was to quickly get it supported in all the tools that currently use this package, in the most frictionless way.I didn't realize this doesn't work for Rollup because it bundles its dependencies, but in other tools now with
already "just works".
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.
Yeah, i'm planning to rename all assertions
references to attributes
, under the new attribute
npm package.
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.
Published acorn-import-attributes@1.9.0.
This PR adds support for the
with
keyword, given that the proposal has recently been changed to https://github.com/tc39/proposal-import-attributes.My goal is to have as few changes as possible (no new package, no AST changes, no major release), so that it will be transparently supported by the rest of the ecosystem:
^1.8.0
of this package ([v3.0] Basic support for import assertions rollup/rollup#4646)Releasing this PR as version 1.9.0 will retroactively add support for import attributes to Webpack and Rollup, helping the community to migrate.
(Btw, where is the code for 1.8.0? It's on npm but not on GitHub)
cc @lukastaegert @TheLarkInn