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 #2230, default identifier renaming issue #2234

Merged
merged 2 commits into from
Jun 4, 2018

Conversation

guybedford
Copy link
Contributor

This ensures that the name of the default variable is kept as its declaration name for function and class declarations, instead of using reference hints.

@diervo
Copy link
Contributor

diervo commented Jun 1, 2018

If we could get a release with this change soon, it will save our team a lot of time :)
@caridy will pay for a dinner in return ;)

@guybedford
Copy link
Contributor Author

@diervo you're welcome. Shameless plug - Salesforce OS support would look great at https://opencollective.com/rollup :)

I don't have publish access, but @lukastaegert should be back today, so we should be able to get this out early this week.

@diervo
Copy link
Contributor

diervo commented Jun 3, 2018

@guybedford @lukastaegert Yes! I'm working on it ;) I just got approval for babel last month (https://opencollective.com/salesforce), and I'm working for getting rollup as well, keep you updated.

Thanks for the quick turn around.

@guybedford
Copy link
Contributor Author

@diervo that would be amazing, thank you so much for looking into it!

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!

console.log('effect');
a = someGlobal;
return a();
}
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the console.log statement as the function call should not be removed even without it. a() is basically equivalent to someGlobal() which can have side-effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

this.name = identifier.name;
if (this.original !== null) {
this.original.addReference(identifier);
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a good approach! The original logic was probably only in place to provide a better name than the file name to unnamed default exports. If we have an id, using that makes a lot of sense.

@lukastaegert lukastaegert added this to the 0.60.0 milestone Jun 4, 2018
@caridy
Copy link

caridy commented Jun 4, 2018

jajaja! Dinner is on me!

@lukastaegert lukastaegert deleted the default-identifier-handling branch June 5, 2018 07:10
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

4 participants