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

added basic support for module attributes and tests updated #10962

Merged
merged 71 commits into from May 24, 2020

Conversation

vivek12345
Copy link
Contributor

@vivek12345 vivek12345 commented Jan 5, 2020

Q                       A
New proposal? https://github.com/tc39/proposal-module-attributes
Patch: Bug Fix? Feature
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

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

import foo from "foo.json" with type: "json";
  • We will be parsing the with keyword and will then parse any custom key value pairs which are passed after the with attribute.
  • In the AST node, we will set node.attributes = [array of these key value pairs]
  • Also we will only be parsing this if the plugin: ["moduleAttributes"] is enabled. otherwise we will raise an error.

We still need to discuss

  • If the attributes do not specify a type attribute, then should we throw an error?
  • If multiple type attributes are present then should we raise an error, as only one type should be allowed?

@nicolo-ribaudo
Copy link
Member

FYI @littledan @xtuc

@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jan 5, 2020
} else if (this.eat(tt._with)) {
return this.parseKeyValuePairs();
}
return [];
Copy link
Member

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:

  1. If there isn't with,
    i. If there is the plugin, return []
    ii. Return null
  2. Expect the plugin
  3. 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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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();
Copy link
Contributor

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.

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

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");
Copy link
Member

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.

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

@vivek12345 vivek12345 Jan 5, 2020

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.

Copy link
Contributor Author

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");

Copy link
Contributor

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

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@JLHwung JLHwung left a 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.

node.shorthand = false;
this.expect(tt.colon);
node.value = this.parsePrimitiveValue();
this.finishNode(node, "ObjectProperty");
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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]

Copy link

@littledan littledan left a 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();

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Thank you @littledan 👍

Copy link
Member

@xtuc xtuc Jan 13, 2020

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?

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.

@JLHwung
Copy link
Contributor

JLHwung commented Jan 8, 2020

@littledan Sure. We are really good at not merging PRs.

@xtuc
Copy link
Member

xtuc commented Jan 13, 2020

Nice work @vivek12345 and thanks for the help @nicolo-ribaudo!

packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
@@ -1459,7 +1488,6 @@ export default class ExpressionParser extends LValParser {
this.next();
return this.finishNode(node, "TemplateLiteral");
}

Copy link
Member

Choose a reason for hiding this comment

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

Nit: undo

packages/babel-parser/src/parser/statement.js Show resolved Hide resolved
@nicolo-ribaudo nicolo-ribaudo added this to the v7.9.0 milestone Jan 14, 2020
@xtuc
Copy link
Member

xtuc commented Jan 15, 2020

I submited #11015 that sketches the AST for Module Attributes

@nicolo-ribaudo
Copy link
Member

@vivek12345 The AST specification PR (#11015) has been merged.

@vivek12345
Copy link
Contributor Author

Understood. I will start making the changes that have been recommended in the reviews along with the AST specification for module attributes.

@xtuc
Copy link
Member

xtuc commented Jan 22, 2020

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?

Vivek Nayyar and others added 2 commits January 25, 2020 17:53
Copy link
Contributor Author

@vivek12345 vivek12345 left a 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.

@vivek12345
Copy link
Contributor Author

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]

Added all these tests to the PR.

this.raise(
node.start,
Errors.ImportCallArity,
this.hasPlugin("moduleAttributes") ? "two arguments" : "one argument",
Copy link
Contributor Author

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.

@vivek12345
Copy link
Contributor Author

@nicolo-ribaudo @JLHwung Is there something else too that needs to be done here or can we merge this now?

@JLHwung
Copy link
Contributor

JLHwung commented May 20, 2020

@vivek12345 I have pushed some changes related to feedback about import(a, b) and trailing comma as we are going to release 7.10 soon. This PR is awesome! Thanks for your contribution.

* 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
*/
Copy link
Member

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!

Copy link
Contributor Author

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 🙏

packages/babel-parser/src/parser/expression.js Outdated Show resolved Hide resolved
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label May 22, 2020
@vivek12345
Copy link
Contributor Author

Thank you @JLHwung . Really happy to see this PR getting merged soon 🎉

@nicolo-ribaudo nicolo-ribaudo merged commit 66b86e0 into babel:master May 24, 2020
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release Spec: Import Attributes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants