Fix Class and FunctionExpressions in default exports#2059
Fix Class and FunctionExpressions in default exports#2059lukastaegert merged 1 commit intorollup:masterfrom danez:class-function-expression
Conversation
|
Thanks for working on a fix here! It could be worth checking if this fix handles the following case as well: // This function gets included
export default (function cube ( x ) {
// rewrite this as `square( x ) * x`
// and see what happens!
return x * x * x;
}, console.log('test'))as that also seems to have the issue from my testing. I'll let @lukastaegert do the full review for this one further. |
|
In the example you posted I think it might be correct to remove // This function gets included
export default (console.log('test'), function cube ( x ) {
// rewrite this as `square( x ) * x`
// and see what happens!
return x * x * x;
}) |
|
@danez ahh I see, yes you're right. I'm wondering if it might be worth looking for a fix where the |
|
Actually forget the above, it would be tricky to get the logic into there. This looks good to me! |
guybedford
left a comment
There was a problem hiding this comment.
Thanks for working on this!
lukastaegert
left a comment
There was a problem hiding this comment.
Thanks for the nice PR! The tests look good, just a minor suggestion where to put the check.
| // export default (function a() {}) | ||
| !isFunctionExpression(this.declaration) && | ||
| !isClassExpression(this.declaration) | ||
| ) { |
There was a problem hiding this comment.
Looking through the code trying to understand the original cause I think the issue is that .referencesOriginal() should never be true for class and function expressions in the first place and I would rather fix the issue there (so that .referencesOriginal() does not "lie"). The problem is that in bindNode(), the presence of a declarationName is misinterpreted as this being a declaration. Then the call to this.variable.setOriginalVariable() triggers this unfortunate effect.
I see two possible approaches
- Only set
declarationNamein.initialiseNode()if this is actually a class or function declaration or an identifier. This has the disadvantage that we do not get the nice name for *expressions. - Check in
.bindNode()if we have a declaration or identifier and callsetOriginal()only in those cases. I would prefer this approach.
There was a problem hiding this comment.
Okay i changed it to use approach 2.
This fixes the problem I described in #2056
I'm not sure if this is a solution you think fits, but as there are only these two cases (Class and Function) I thought it might be feasible to directly check for them.
Let me know what you think.
In any way feel free to come up with a better solution and use my tests :)