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

Fix for transforming methods in ES6 object shorthand form #4

Merged
merged 3 commits into from
Jan 18, 2015

Conversation

jareware
Copy link
Owner

Since the previous release there's a regression in transforming the form:

var o = {
    someMethod() /*: boolean */ {}
};

By a quick look it seems that leadingComments are attached to the FunctionExpression, as opposed to the BlockStatement body of the function.

Added a failing test case.

@jareware
Copy link
Owner Author

...which fails locally but not on Travis.

@jareware
Copy link
Owner Author

Okay, Travis build fixed in c6d0d2f. Rebasing this PR in a bit.

@jareware
Copy link
Owner Author

/cc @phadej :)

@phadej
Copy link
Contributor

phadej commented Jan 18, 2015

Is this some kind of ES6 shortcut, for defining object properties, which are functions?

@jareware
Copy link
Owner Author

Exactly that, I think they're called "methods".

Actually, seems that within processFunctionNode(), node.parent.method === true in that case, but not in case of a traditional "function as object property".

I'll look into this more in a bit...

…e there's no regressions in their parsing in the presence of "ES6 methods".
jareware added a commit that referenced this pull request Jan 18, 2015
Fix for transforming methods in ES6 object shorthand form
@jareware jareware merged commit 56c5ca3 into master Jan 18, 2015
@jareware jareware deleted the bug-object-shorthand branch January 18, 2015 21:45
.replace(/\/\*\s*:([\s\S]+?)\*\//g, ': $1'); // /* : FooBar */ => : FooBar

// If this is an "ES6 method" function, its return-type annotation is included as leadingComments
if (node.parent.method && node.leadingComments) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is in between of node's header updating

jareware added a commit that referenced this pull request Jan 19, 2015
@jareware
Copy link
Owner Author

Fair enough!

Let's fix it while fixing this #6...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants