-
-
Notifications
You must be signed in to change notification settings - Fork 258
ClassFields: Allowing comma separated grammar #608
Conversation
src/parser/statement.js
Outdated
this.state.inCommaSeparatedFields = true; | ||
// Copy decorators for cases like `@foo x, y;` | ||
if (decorators.length) { | ||
member.decorators = decorators; |
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.
Let's slice here. It'll be really weird if a transform pushes a decorator and two fields get it.
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.
Good call.
src/parser/statement.js
Outdated
@@ -899,7 +918,7 @@ export default class StatementParser extends ExpressionParser { | |||
} | |||
} | |||
|
|||
if (decorators.length) { | |||
if (!this.state.inCommaSeparatedFields && decorators.length) { |
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 understand why inCommaSeparatedFields
affects this. Maybe it should be a separate check instead (if fields can't have a trailing comma)?
src/parser/statement.js
Outdated
@@ -1071,7 +1090,7 @@ export default class StatementParser extends ExpressionParser { | |||
/* isConstructor */ false, | |||
); | |||
this.checkGetterSetterParamCount(method); | |||
} else if (this.isLineTerminator()) { | |||
} else if (this.isLineTerminator() || this.match(tt.comma)) { |
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.
Does this mean methods can have trailing commas?
class Foo {
method() {
},
}
I don't see a test for it anywhere.
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.
Will add more tests.
src/parser/statement.js
Outdated
|
||
if (isClassField) { | ||
if (this.eat(tt.comma)) { | ||
this.state.inCommaSeparatedFields = true; |
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 can probably avoid this state entirely.
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 we can completely get away without knowing we are within the context of a coma separated production due to ASI and explicit checks for ;
Im revising it nonetheless.
Why? Usually the semicolon is included: var a = 2; // start: 0, end: 10 foo(); // start: 0, end: 6 |
@nicolo-ribaudo Not for comma separated statements, which is what I'm refering here. import { a, b, c } from "x" // location do not include comma
var x = 1, b = 2, z = 3; // location do not include comma Will revisit, and will try to to make it consistent, but it might be tricky. |
- Fixes babel#540 - Allow decorators for comma separated fields - Added tests
113ea5a
to
8dcb23b
Compare
Ok based on the comments I refactor pretty much the whole thing (sorry I had to rebase for simplicity of sake):
@jridgewell We need to know whether or not we are in a "comma-separated" statement inside |
I have a doubt about decorators: class A {
@dec x, y;
}
class B {
@dec x;
y;
}
class C {
@dec x;
@dec y;
} Is |
@peey should revisit this logic once this PR lands in master. |
@diervo Agree with what you're saying about equivalency (except it matters how many times Some test ideas:
cc @bakkot |
Then the AST must somehow reflect the difference between a decorator applied to a list of properties and the same decorator applied to different properties. Otherwise how could e.g. ASTProgram {
body: [
ClassDeclaration {
id: Identifier { name: "Foo" },
body: ClassBody {
body: [
ClassProperty {
decorators: [
Decorator {
expression: CallExpression {
callee: Identifier { name: "dec" },
arguments: []
}
}
],
key: Identifier { name: "a" }
},
ClassProperty {
decorators: [
Decorator {
expression: CallExpression {
callee: Identifier { name: "dec" },
arguments: []
}
}
],
key: Identifier { name: "b" }
}
],
},
},
FunctionDeclaration {
params: [],
body: BlockStatement {
body: [
"dec = () => { throw new Error(); }",
"return /* super cool decorator */"
]
}
}
]
} This ast represents to different programs: one works and the other throws. class Foo {
@dec() a, b; // works
}
function dec() {
dec = () => { throw new Error(); };
return decorator;
} class Foo {
@dec() a;
@dec() b; // Throws
}
function dec() {
dec = () => { throw new Error(); };
return decorator;
} A possible solution is to represent a comma-separated list of properties actually as a list of properties, similar to a class Foo {
@dec() a, b;
} ASTClassDeclaration {
id: Identifier { name: "Foo" },
body: ClassBody {
body: [
ClassPropertiesList {
decorators: [
Decorator {
expression: CallExpression {
callee: Identifier { name: "dec" },
arguments: []
}
}
],
static: false,
properties: [
ClassProperty {
key: Identifier { name: "a" }
},
ClassProperty {
key: Identifier { name: "b" }
}
],
}
],
},
} This approach has other two possible little advantages:
I'm sorry if I may sound quite pedantic, but I'm trying to highlight possible problems 😶 |
@littledan @nicolo-ribaudo I think you are both right, but this will require some interesting refactor, and probably a somewhat big breaking change on the transformers for publicFields. Is everyone ok then moving to the following productions (analogous to
Basically any public/private fields will be enclosed by FieldDefinition, and no more decorators in the Fields itself. If we do this, we might as well refactor the whole ClassBody to make it future proof as @jridgewell was suggested to do in #609. If you give me thumbs up I can give it a run today, otherwise will have to wait til next weekend most likely or someone else can take over. Thoughts? |
@nicolo-ribaudo The problem is not creating a new plugin, is the fact that we are changing the shape of the Nodes (for example no longer ClassProperty will have decorators, or potentially a change on the node name ClassProperty =>PublicField ) that transformers expect. And I'm very reluctant to have a fork in the code that generate different Node types based on the syntax plugin that is enabled, specially since we are working in babel7 where we have more room for error. If we do this changes, my vote is to do it in a non backward compatibility way. @hzoo what you think? |
If this is for Babel 7, could the legacy decorator syntax and transform also be updated to the new AST as well? It seems like the new form you're proposing is simply more general. Are there a lot of transforms out there which depend on the current shape of decorators? |
Closing this since comma separated proposal was dropped from the original classField proposal and will be advancing separately. |
If someone wants to reopen the PR and pick it up, you can find the new proposal at https://github.com/littledan/proposal-comma-separated-fields . The default class fields flag needs to prohibit comma separation, with a separate flag to permit it; I am not sure whether this flag behavior should be implemented at the babylon or transform level. |
Refactor for classFields: