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
Conversation
FYI @littledan @xtuc |
} else if (this.eat(tt._with)) { | ||
return this.parseKeyValuePairs(); | ||
} | ||
return []; |
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 think that we should do something like this:
- If there isn't
with
,
i. If there is the plugin, return[]
ii. Returnnull
- Expect the plugin
- return
parseKeyValuePairs
By doing so, we don't return an empty array if the plugin is disabled (since disabled plugins shouldn't have any effect on the AST).
Even better, we should only set node.attributes
if maybeParseModuleAttributes
returns the array.
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.
Understood. That makes sense. I will make the changes accordingly.
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.
updated the code with these changes.
node.properties = []; | ||
|
||
do { | ||
const prop = this.parseObjectMember(); |
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.
It maybe overkill to use parseObjectMember
here because it is likely to include only primitive types (see tc39/proposal-import-attributes#22 (comment)), i.e.
import foo from "foo" with loaders: ["bar", "qux"]
are not allowed because array is not a primitive type. As a stage-1 proposal, I suggest we implement the minimal superset of proposal. Should the proposal accepts more types, we can always drops restrictions later.
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.
Understood. I will write a simple parseKeyValue function. Thank you 👍
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.
Refactored with the suggested changes
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.
We aren't sure if we want to allow any value there, let's keep it simple for now I think; only stringliterals
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.
Made the changes and now we are only allowing string literals.
if (!this.match(tt.eq) && propHash.start !== undefined) { | ||
this.raise(propHash.start, "Redefinition of __proto__ property"); | ||
} | ||
return this.finishNode(node, "ObjectExpression"); |
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 module attributes allow arbitrary values (I might be wrong).
I think that this should be simplified to (pseudo-JavaScript)
const attrs = [];
do {
const node = startNode();
parseIdentifier();
expect(":");
parseLiteral();
} while (eat(","));
return attrs; // Return the array of attributes, not a wrapper node.
It could also be inlined in maybeParseModuleAttributes
, since it's short.
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 looks way better and simpler. I will make the changes. Thank you for the inputs 👍
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.
few questions here, should the above implementation look something like this
const attrs = [];
do {
const node = this.startNode();
node.key = this.parseIdentifier();
node.computed = false;
node.method = false;
node.shorthand = false;
this.expect(tt.colon);
node.value = this.parseLiteral(this.state.value);
this.finishNode(node, "ObjectProperty");
attrs.push(node);
} while (this.eat(tt.comma));
One thing that I am able to notice here is that parseLiteral would also need a type value which I need to deduce somehow?
I don't think I should be using a parseExprAtom
for this, so should I write a parsePrimitiveValue
function which will internally based on the type pass the correct node type.
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.
or should we just do
node.value = this.parseLiteral(this.state.value, "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.
The second type
argument is meant to set type of the AST nodes. Babel supports the following types
(but not general "Literal"
)
RegExpLiteral
NullLiteral
StringLiteral
BooleanLiteral
NumericLiteral
BigIntLiteral
A primitive value is a member of one of the following built-in types: Undefined, Null, Boolean, Number, String, and Symbol. So you can guard against parseExprAtom
with (pseudoJS)
this.match(tt._null) || ... tt._true, tt._false, tt.name (and value is "undefined"), tt.string, tt.num, tt.bigint
Note that Symbol
is also a primitive type and not covered above, but I think we can forbid symbol in module attributes? @xtuc
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.
These changed have been updated. Thank you
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'd recommend starting with just supporting string literals at this point.
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.
Made the changes and now we are only supporting String Literals.
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.
Nice work! Note that we have to add type definitions to babel-types and generator supports, too.
…refactored the parsing logic
node.shorthand = false; | ||
this.expect(tt.colon); | ||
node.value = this.parsePrimitiveValue(); | ||
this.finishNode(node, "ObjectProperty"); |
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 new ModuleAttribute
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
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.
Can you also add the following tests, to check ASI?
import "x" with type: "json"
[0]
import "x"
with
type: "json"
This one should throw either about with
in strict mode, or about the (
where we expect an identifier.
import "x"
with ({});
PS: It looks like you commits are not linked to your account. You can run
git config user.email [your email used for github]
or
git config --global user.email [your email used for github]
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 asked me offline about what sort of verification should be done on module attributes. I want to discuss this question further with the module attributes champion group before getting back to you with a full answer. We have a call next week and I'll get this question on the agenda. So, I'd recommend against merging this PR until that happens.
node.method = false; | ||
node.shorthand = false; | ||
this.expect(tt.colon); | ||
node.value = this.parsePrimitiveValue(); |
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 introduce parsePrimitiveValue
.
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:
import u from "a" with lazy: true, startAtLine: 1;
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.
@littledan Sure. We are really good at not merging PRs. |
Nice work @vivek12345 and thanks for the help @nicolo-ribaudo! |
@@ -1459,7 +1488,6 @@ export default class ExpressionParser extends LValParser { | |||
this.next(); | |||
return this.finishNode(node, "TemplateLiteral"); | |||
} | |||
|
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
I submited #11015 that sketches the AST for Module Attributes |
@vivek12345 The AST specification PR (#11015) has been merged. |
Understood. I will start making the changes that have been recommended in the reviews along with the AST specification for module attributes. |
To follow up here. We are planning to restrict the module attributes to StringLiteral only, for now. Is there anything I can do to help @vivek12345? |
…-module-attributes
…ted and only allow string literals for attribute values
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 everyone for the reviews. I have updated the code with the changes.
@xtuc I have made the changes to only allow String Literals for now.
Added all these tests to the PR. |
this.raise( | ||
node.start, | ||
Errors.ImportCallArity, | ||
this.hasPlugin("moduleAttributes") ? "two arguments" : "one argument", |
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.
we are passing a message here to explain the exact number of arguments needed. for module attributes it is two arguments and for every other thing it is only one argument.
@nicolo-ribaudo @JLHwung Is there something else too that needs to be done here or can we merge this now? |
@vivek12345 I have pushed some changes related to feedback about |
* but if we do have a default import | ||
* we need to check if we have a comma after that and | ||
* that is where this `|| this.eat` condition comes into play | ||
*/ |
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 🙏
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
Thank you @JLHwung . Really happy to see this PR getting merged soon 🎉 |
This change is to let the babel parser parse the custom module attributes as discussed in the proposal above.
Since it is a stage-1 proposal we will definitely have a lot more changes coming up but this is an early draft of what the implementation would look like.
Now we can specify module attributes in the following way
with
keyword and will then parse any custom key value pairs which are passed after thewith
attribute.plugin: ["moduleAttributes"]
is enabled. otherwise we will raise an error.We still need to discuss
type
attribute, then should we throw an error?type
attributes are present then should we raise an error, as only one type should be allowed?