-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
What I mean is, what if an identifier has the value The assumption Edit: Looks very dry and nice otherwise. |
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 |
I fear the best thing to do is to skip-identifiers. It's unlikely to end up with Say our |
agreed |
@Rich-Harris Mind if I go ahead and merge this? |
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) |
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:
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).