Skip to content

Prevent transform-es2015-function-name from altering MethodDefinition's Re: T6779#3149

Closed
jmm wants to merge 2 commits intobabel:masterfrom
jmm:T6779-func-name-method
Closed

Prevent transform-es2015-function-name from altering MethodDefinition's Re: T6779#3149
jmm wants to merge 2 commits intobabel:masterfrom
jmm:T6779-func-name-method

Conversation

@jmm
Copy link
Member

@jmm jmm commented Dec 9, 2015

See https://phabricator.babeljs.io/T6779. Looking for feedback on whether this is:

  1. The correct outcome.
  2. A sensible way to get there. With the current implementation, I guess it was more efficient to visit the ObjectExpression's and loop all the props / methods, but if just the props are being considered it seems like visiting ObjectProperty's makes sense.
  3. An object literal (ObjectExpression) is the only context where an ObjectProperty can appear.

My understanding is that transforming an ES2015 object literal method definition (e.g. {x () {}}) to ES5 has at least one important semantic limitation (regarding super -- see the issue). (Someone please confirm.) So it seems to me it ought to be possible to do this:

input.js

// ES2015 that should not be transformed.
var x = {x () { super.x(); }};

// ES5 that should get transformed to have a name.
var y = {y: function () {}};
babel.transform(input, {
  plugins: ["babel-plugin-transform-es2015-function-name"]
});

My understanding is that babel-plugin-transform-es2015-shorthand-properties is the one that should transform ES2015 like in this example and it seems to me that the user should opt-in to that if they want to transform the method definition syntax and accept the limitations. It should be possible to use both transforms together if you want the outcome that babel-plugin-transform-es2015-function-name on its own is delivering currently with this input, correct? (Actually, now that I think about it, should shorthand-properties not automatically name the function when transforming MethodDefinition's?)

Existing tests pass with this change, so if this is not the correct behavior the actual fixture ought to be adopted along with a different expected.

jmm added 2 commits December 8, 2015 16:46
@jmm
Add method definition fixtures.
24998bb 
(Failing.)
@jmm
Apply only to ObjectProperty.
36e58f4 
Fix T6779. It should be possible to apply this to ES5 functions without
mutating ES2015 MethodDefinition's.
@codecov-io
Copy link

codecov-io commented Dec 9, 2015

Current coverage is 84.65%

Merging #3149 into master will decrease coverage by -0.26% as of 36e58f4

@@            master   #3149   diff @@
======================================
  Files          214     214       
  Stmts        15598   15580    -18
  Branches      3330    3325     -5
  Methods          0       0       
======================================
- Hit          13245   13190    -55
- Partial        677     716    +39
+ Missed        1676    1674     -2

Review entire Coverage Diff as of 36e58f4

Powered by Codecov. Updated on successful CI builds.

@hzoo
Copy link
Member

hzoo commented Dec 9, 2015

Ok forgot about super in object literals - right it does look like that has to be in a ObjectMethod.

And babel-plugin-transform-es2015-shorthand-properties already converts both objectproperty/objectmethod so it looks like a good idea to split the two and not have overlapping functionality. 👍

@jmm
Copy link
Member Author

jmm commented Dec 9, 2015

@hzoo Cool, thanks for reviewing.

And babel-plugin-transform-es2015-shorthand-properties already converts both objectproperty/objectmethod so it looks like a good idea to split the two and not have overlapping functionality.

Ironically, since I started this it occurred to me that maybe shorthand-properties should be doing the naming on ObjectMethod's itself. I'll probably bring that up in slack or another issue.

@amasad
Copy link
Member

amasad commented Dec 10, 2015

I don't understand the issue with regards to super? What is the expected output? To not transform to function at all?

@jmm
Copy link
Member Author

jmm commented Dec 10, 2015

@amasad Thanks for reviewing.

I don't understand the issue with regards to super?

Sorry, that's my fault. The super part is actually misstated in the bug report and in my followups. Thanks for your comment -- it prompted me to think the situation through more and take a closer look. The bug report misstates the problem by saying it's:

> not valid code due to super out of method.

But the real problem there is that it's super in ES5, right? I think the problem the bug report raises is solved by adding babel-plugin-transform-es2015-object-super.

However, the report brought this other issue to my attention and I still think the outcome of this PR is correct.

EDIT: it's not so much that the issue report misstates the problem -- as the title says, I think the OP's main point is that it shouldn't transform the MethodDefinitions to functions, so I think this PR is what they're asking for. Re: the super part, the more immediate problem for most people would be that super isn't valid ES5 period (I believe). If the topic is ES6 super syntax as the output target, then it's a question of where it can appear, and my understanding is that the output in the report is still invalid in that case.

What is the expected output? To not transform to function at all?

Correct, that's what I'm proposing. As in the fixtures here, MethodDefinition syntax input and output. Forget super completely. The subject of this PR is: should it be possible to simultaneously use transform-es2015-function-name to mock ES2015 function name inference for anonymous functions (ES5), while not transforming MethodDefinition's? I think yes.

Those seem like separate concerns to me and my understanding is that transform-es2015-shorthand-properties is the one that should be concerned with MethodDefinition. (I'm also going to submit a PR proposing that that always transform them to named functions since I think that more closely approximates the real ES2015 semantics.)

The hypothetical is enough, but recent versions of V8 (Chrome 45 / Node 5) are a perfect example of why I think this behavior makes sense:

var x = {
  y: function () {},
  z () {},
};

console.log(x.y.name); // ""
console.log(x.z.name); // "z"

@amasad
Copy link
Member

amasad commented Dec 16, 2015

Cool, can you cite the ES2015 to make sure we're doing the right thing? Browsers often don't fully implement the spec.

@jmm jmm closed this Dec 17, 2015
@jmm jmm reopened this Dec 17, 2015
@jmm jmm closed this Dec 17, 2015
@jmm jmm mentioned this pull request Dec 17, 2015
Merged
@jmm
Copy link
Member Author

jmm commented Dec 17, 2015

@amasad I'm sorry, I muddled this by talking about super, which really isn't relevant here, and other stuff outside the scope of this PR. Rather than heavily edit the description here I started over with a new PR with a focused description: #3176. In a nutshell, there's no spec-correctness question involved in this PR. The PR only addresses this question: should you be able to opt-in to naming anonymous functions and not transform method definitions at the same time.

@jmm jmm deleted the T6779-func-name-method branch January 19, 2016 22:16
@babel-bot babel-bot mentioned this pull request Sep 7, 2016
Closed
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmm @codecov-io @hzoo @amasad