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

exclude dead branches #249

Merged
merged 5 commits into from
Nov 11, 2015
Merged

exclude dead branches #249

merged 5 commits into from
Nov 11, 2015

Conversation

Rich-Harris
Copy link
Contributor

Follow-up to #208 (comment). This PR excludes dead branches, wherever it's possible to determine that through static analysis.

This goes slightly further than what e.g. UglifyJS does:

var obj = {};
obj.foo = function () {
  console.log( 'this function should be excluded' );
};

if ( false ) obj.foo();

Rollup will exclude obj.foo but UglifyJS won't, as far as I can tell (there may be an option that I don't know about).

Alongside https://github.com/rollup/rollup-plugin-replace, this should make it easier to make different bundles for different targets (e.g. browser vs node, dev vs prod).

function nodesAreNotEqual ( a, b ) {
if ( a.type !== b.type ) return false;
if ( a.type === 'Literal' ) return a.value != b.value;
if ( a.type === 'Identifier' ) return a.name != b.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to use != here while you use === above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's to do with strict vs non-strict equality. though now that you mention it, i think it'd be better to have separate nodesAreStrictEqual and nodesAreNotStrictEqual functions

@Victorystick
Copy link
Contributor

What I mean is, what if an identifier has the value NaN?

The assumption a == a -> true is invalid.

Edit: Looks very dry and nice otherwise.

@Rich-Harris
Copy link
Contributor Author

What I mean is, what if an identifier has the value NaN?

Ah. Good point. Bloody NaN! Is there any more useless feature in any programming language?

So do we skip identifier equality checks altogether? I guess the only alternative is to have an option controlling whether or not to care about NaN, which is territory I'd rather not get into

@Victorystick
Copy link
Contributor

I fear the best thing to do is to skip-identifiers. It's unlikely to end up with NaNs like that, but it could happen. Unless we can be sure of the identifiers' values.

Say our Declarations had a value property, then we could compare actual values. const variables would be easy. It'd be a best-effort thing regardless.

@Rich-Harris
Copy link
Contributor Author

agreed

@Victorystick
Copy link
Contributor

@Rich-Harris Mind if I go ahead and merge this?

@Rich-Harris
Copy link
Contributor Author

Yep let's do it. We should probably merge #253 too and release it as 0.21 (so that people can pin 0.20 if it turns out the algorithm fails in certain scenarios)

Rich-Harris added a commit that referenced this pull request Nov 11, 2015
@Rich-Harris Rich-Harris merged commit 9099c49 into master Nov 11, 2015
@Rich-Harris Rich-Harris deleted the dead-branches branch November 11, 2015 22:39
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.

2 participants