Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
0621f12 to
5dd4f7c
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/26584/ |
5dd4f7c to
a4578f4
Compare
| }, | ||
| "computed": false, | ||
| "kind": "set", | ||
| "variance": null, |
There was a problem hiding this comment.
The variance property exists in ClassProperty, and we don't have variance for ObjectMethod. This PR removes variance of fooProp accessor for the following cases:
a={set fooProp(value:number){}}I think the new behaviour is preferred and it aligns to what we did for non-accessors.
| prop.static = false; | ||
| classBody.body.push(this.parseClassProperty(prop)); | ||
| return true; | ||
| } else if (containsEsc) { |
There was a problem hiding this comment.
This branch is unreachable because parseClassMemberFromModifier is guarded by isContextual call, which must return false when containsEsc.
| ) { | ||
| // https://tc39.es/ecma262/#prod-AsyncMethod | ||
| // https://tc39.es/ecma262/#prod-AsyncGeneratorMethod | ||
| if (prop.key.name === "async" && !this.hasPrecedingLineBreak()) { |
There was a problem hiding this comment.
Parsing async and set, getter have been unified in parsePropertyDefinition -- so they can share same sanity checks. Thus we don't need passing containsEsc (A tokenizing level states) to parseObjectMethod, instead isAccessor is pass.
|
|
||
| if (this.prodParam.hasYield && this.eat(tt.dot)) { | ||
| if (this.prodParam.hasYield && this.match(tt.dot)) { | ||
| const meta = this.createIdentifier( |
There was a problem hiding this comment.
Avoid creating an extra meta node when tt.dot is not met, instead we create meta when we have to. The same technique is also applied for parseNewOrNewTarget.
| @@ -594,163 +602,205 @@ export default class ExpressionParser extends LValParser { | |||
| state: N.ParseSubscriptState, | |||
There was a problem hiding this comment.
parseSubscript has been broken into smaller pieces. I find it more easier to go through after refactoring: https://github.com/JLHwung/babel/blob/ee4d63ef00e5090f44ffedee718dd18fdc6e21e0/packages/babel-parser/src/parser/expression.js#L597
|
I have changed the PR label because the bug fixed here is edging cases. It is more of a refactoring which is very likely invisible to users. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
I left a few nits, but overall I find that this improves the readability 👍
| startLoc = startLoc || this.state.startLoc; | ||
| startPos = startPos || this.state.start; | ||
| left = left || this.parseBindingAtom(); | ||
| startLoc = startLoc ?? this.state.startLoc; |
There was a problem hiding this comment.
Maybe now we can use ??=? 😄
Flow 😞
|
|
||
| parseMaybeUnary(refExpressionErrors: ?ExpressionErrors): N.Expression { | ||
| // https://tc39.es/ecma262/#prod-UnaryExpression | ||
| parseUnary(refExpressionErrors: ?ExpressionErrors): N.Expression { |
There was a problem hiding this comment.
I think that this was called parseMaybeUnary (like parseMaybeConditional, parseMaybeAssign) because it could return something which is not a unary expression, like a simple identifier.
I know that technically Identifier is a child of UnaryExpression in the spec, but it's just a side effect of how it's specified.
206786e to
1d693d8
Compare
Refactors
@babel/parsera bit. Focused on code readability and syntax upstream spec links.Happens to fix a bug when adding grammar production comments.