Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added basic support for module attributes and tests updated #10962
added basic support for module attributes and tests updated #10962
Changes from 2 commits
7e1a783
b810e31
c35b06e
4243ec1
202af5a
14aae84
8c4ecf9
bbb6020
ba5482e
a038601
cde4f92
4f16179
a264662
c7c2c78
2b5b437
d36d314
0a0958d
f28f6ad
7806809
a3c6a55
65f9f5c
1dacc99
187d0d6
5cb50ed
40ddfab
41bb994
4030426
792d096
a3c56ff
cbcdb3d
e392984
256510b
1ee26a7
2a708a5
f795d36
35a85e9
e3976f2
efacdaf
4202fa9
e1016ad
f89c15f
18dd064
31f6a38
b98824f
1c2be01
180b757
ca2608f
3581a82
b9d2939
24f7830
b1270c6
b7280e2
6e4e730
7a2f4f8
69f153e
522bdc6
2a3e3d0
b1e7ddc
2e243af
f250937
dbf36f6
0ce6e62
9f80885
aa22f1a
967a9be
d06d08b
f051fa1
01933e4
dde6294
907aacd
63b7c44
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: undo
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.
@vivek12345 I don't know if I already thanked you, but I love these comments!
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.
Thank you @nicolo-ribaudo . Means a lot 🙏
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.
Nit: can we do something like this?
By doing so, we throw the "missing plugin" error pointing at
with
rather than at what comes after.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.
This makes sense. I will update the code 👍
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.
To align with Property Name, the identifier name should be liberal, that is,
should be valid.
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.
IMO
this.parseLiteral(this.state.value, "StringLiteral");
would be sufficient; then, you don't need to introduceparsePrimitiveValue
.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.
Noted. Thank you @littledan 👍
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.
I agree with @littledan
That said, we didn't define the grammar yet but we might want to consider things like:
instead of a string literal?
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, we might want to consider it. But I don't think this is worth complicating the Babel codebase at the moment.
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.
I don't think that we should re-use
ObjectProperty
: it's conceptually and syntactically different. We should introduce a newModuleAttribute
node.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.
Would it help if we agreed on the Babel AST before the implemation?
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.
Changed the code to now use the new AST node
ImportAttribute