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

Directive handling #1856

Merged
merged 5 commits into from
Jan 12, 2018
Merged

Directive handling #1856

merged 5 commits into from
Jan 12, 2018

Conversation

porcellus
Copy link
Contributor

Improving directive handling, as discussed in #1855

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.

LGTM

@@ -0,0 +1,5 @@
export var count = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I assume this file can be deleted? 😉

@lukastaegert
Copy link
Member

Also, very nice tests! I am just doing some final checks before merging 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.

Just one small adjustment needed to avoid repeated warnings, otherwise really nice.

shouldBeIncluded() {
if (this.directive && this.directive !== 'use strict') {
if (this.parent.type === "Program") {
this.module.warn( // This is necessary, because either way (deleting or not) can lead to errors.
Copy link
Member

Choose a reason for hiding this comment

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

shouldBeIncluded is run for each tree-shaking pass, which means the error is displayed at least twice, possibly more often. The rest of the code looks nice and I like the approach but the proper place to display the warning is to add a member initialiseNode, cf. e.g. the warning in ThisExpression. initialiseNode is run only once and is meant for node specific initialisations that do not depend on the scope being populated.

@alippai
Copy link
Contributor

alippai commented Jan 11, 2018

@lukastaegert are you planning releasing this feature or should I expect some delay (I see there is a nice progress on other PRs and issues)

@lukastaegert
Copy link
Member

I plan on releasing this together with #1816 and possibly others maybe as early as tomorrow. It was just a little late for the previous release 😜

@alippai
Copy link
Contributor

alippai commented Jan 11, 2018

Sure, awesome! Keep up the amazing work

@lukastaegert lukastaegert added this to the 0.54.0 milestone Jan 12, 2018
@lukastaegert lukastaegert merged commit 13c779d into rollup:master Jan 12, 2018
@porcellus porcellus deleted the directive_handling branch January 12, 2018 10:52
@alippai
Copy link
Contributor

alippai commented Jan 12, 2018

@lukastaegert thanks!

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