Skip to content

Fix Class and FunctionExpressions in default exports#2059

Merged
lukastaegert merged 1 commit intorollup:masterfrom
danez:class-function-expression
Mar 17, 2018
Merged

Fix Class and FunctionExpressions in default exports#2059
lukastaegert merged 1 commit intorollup:masterfrom
danez:class-function-expression

Conversation

@danez
Copy link
Contributor

@danez danez commented Mar 14, 2018

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 :)

@guybedford
Copy link
Contributor

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.

@danez
Copy link
Contributor Author

danez commented Mar 14, 2018

In the example you posted I think it might be correct to remove cube as it has no side-effects and is unsed. The exported expression is the last in sequence (console.log) if I'm not mistaken.
For example swapping the two expressions around leaves them both in the bundle correctly:

// 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;
})

@guybedford
Copy link
Contributor

guybedford commented Mar 14, 2018

@danez ahh I see, yes you're right.

I'm wondering if it might be worth looking for a fix where the ExportDefaultVariable never passed referencesOriginal() by moving the logic into there? I believe this logic is attempting to tell when export default x is basically identical to export { x as default } so that it can be removed. So in this case it isn't, as it is itself the original (which is unique to this case).

@guybedford
Copy link
Contributor

Actually forget the above, it would be tricky to get the logic into there. This looks good to me!

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

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)
) {
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. Only set declarationName in .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.
  2. Check in .bindNode() if we have a declaration or identifier and call setOriginal() only in those cases. I would prefer this approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i changed it to use approach 2.

@lukastaegert lukastaegert added this to the 0.57.1 milestone Mar 17, 2018
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Looks good!

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.

3 participants