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

Static analysis to eliminate dead code #1061

Closed
jknight12882 opened this issue Oct 18, 2016 · 6 comments
Closed

Static analysis to eliminate dead code #1061

jknight12882 opened this issue Oct 18, 2016 · 6 comments

Comments

@jknight12882
Copy link

jknight12882 commented Oct 18, 2016

webpack is able to do static analysis on the following UMD code to remove it while rollup leaves it behind.

  // UMD wrapper.
  /*global define:false*/
  if (typeof exports === "object" && typeof module === "object") {
    // CommonJS
    module.exports = _lload;

  } else if (typeof define === "function" && define.amd) {
    // AMD
    define([], function () { return _lload; });

  } else {
    // VanillaJS
    window._lload = _lload;
  }

this should be replace by

if (true) {
    // CommonJS
    module.exports = _lload;
    ...

which can then be eliminated

@Rich-Harris
Copy link
Contributor

This is a two-parter – first, the commonjs plugin would need to rewrite the typeof expressions. Just opened rollup/rollup-plugin-commonjs#151 for that.

Second, Rollup's dead code elimination would need to handle logical expressions in if block conditions. I thought it did, but apparently not – this works, but this doesn't.

Rich-Harris added a commit that referenced this issue Dec 29, 2016
statically analyse LogicalExpression nodes
@Rich-Harris
Copy link
Contributor

Fixed in 0.38.3 and [email protected]

@kzc
Copy link
Contributor

kzc commented Dec 29, 2016

Great stuff.

I guess the next logical step would be to add another bit of functionality to remove curly braces around blocks without let and const:

{
	console.log( 'true' );
}

Or perhaps just leave that to the downstream minifier to deal with.

@Rich-Harris
Copy link
Contributor

Yeah, I think Rollup should ideally remove the braces. Minifiers will remove them, so it's not a super high priority, but it does look weird if they're not removed. Not just let and const, I'm fairly sure class and function declarations are also block-scoped.

@kzc
Copy link
Contributor

kzc commented Dec 29, 2016

So just a matter of descending the block AST and if those 4 constructs are not found then the braces can be removed?

@Rich-Harris
Copy link
Contributor

Think so yeah. Wouldn't even need to walk the BlockStatement since any declarations would have to be direct descendants of it, I think, so off the top of my head:

const blockScopedDeclarations = block.children.filter( node =>
  /Declaration/.test( node.type ) && node.kind !== 'var' );

if ( blockScopedDeclarations.length === 0 ) {
  // remove curlies. for extra credit, deindent...
}

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

No branches or pull requests

3 participants