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
Add ES6 class definitions #19
Conversation
type: "MethodDefinition"; | ||
key: Identifier; | ||
value: FunctionExpression; | ||
kind: "init" | "get" | "set"; |
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.
@sebmck Isn't it ""
for regular methods in current implementation? "init"
seems to make sense for Property
only (where you actually initialize it with some value), however I'm more than open to specifying "method"
or smth here.
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.
Ah, yes it is. Copy+paste from Property
fail.
```js | ||
interface ClassBody <: Node { | ||
type: "ClassBody"; | ||
body: [ MethodDefinition ]; |
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.
How about renaming this to methods
instead? For BlockStatement
, we can't change it, but here I think we could fix the classStmt.body[0].body[1].value
chain into smth more meaningful.
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.
Possibly. Just wanted to align this PR with the current pseudo-standardised behaviour.
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.
Hesitant to rename it considering it's already interoperable and this seems like a minor nit.
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.
Sure, thanks for that, just trying to bring some discussion before we get this as standardized behavior.
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.
Yeah, should always discuss ahead of time. Glad you're reviewing these PRs ;-)
So is the consensus that this is good to merge since it documents the current pseudo-standardised behaviour of Acorn/Esprima and changes can be made and discussed in separate PRs? |
Yes. |
👍 |
What's the process going to be for merging pull requests and agreeing on changes? |
@sebmck, sounds like if there is already interop between acorn, SpiderMonkey, and Esprima, we should just document, and have 1 person from each give a plus one. In case of proposed modifications, I'd imagine a discussion period on the Issue followed by plus one from each SpiderMonkey, Esprima, and acorn. If we can't agree on a proposed modification, we should at least document the differences and attempt at a later date to resolve. @sebmck, @dherman, sound ok? If so, we should document this process. It's a bit suboptimal because it ignores other potential implementations, so I'm open to suggestions. |
I think that's a good starting point, and it's all on GH so it's open participation, but if people feel we are moving too fast we can adjust as we go. |
@mikesherov Yeah, documenting it sounds good, there shouldn't be any ambiguity with these types of decisions. |
Both acorn and esprima harmony follow this structure.