Skip to content

Treeshake sequence expression - #1650

Merged
lukastaegert merged 2 commits into
rollup:masterfrom
corneliusweig:treeshake-sequence-expression
Oct 6, 2017
Merged

Treeshake sequence expression#1650
lukastaegert merged 2 commits into
rollup:masterfrom
corneliusweig:treeshake-sequence-expression

Conversation

@corneliusweig

Copy link
Copy Markdown
Contributor

This resolves #1649

The resulting code may have spurious parentheses that wrapped the sequence. For example var a = (1,2,3) becomes var a = (3) instead of var a = 3. Other than that, all cases covered in #1649 are covered.

- if the test evaluation produces an effect, keep the whole expression
- if the test evaluation is unkown but has no effect, remove the expression unless any branch has an effect
- if the test evaluation is known, replace the expression with the resulting branch (as before)
@kzc

kzc commented Sep 27, 2017

Copy link
Copy Markdown
Contributor

Is #1648 a superset of this PR?

@lukastaegert lukastaegert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really like this! No objections to merging this.

@lukastaegert

Copy link
Copy Markdown
Member

@kzc It looks like it but from my side we should merge both anyway.

@corneliusweig

Copy link
Copy Markdown
Contributor Author

@kzc no, #1648 only deals with the ConditionalExpression. There is a small conflict between #1650 and #1648 so I can provide a combined PR if you like. It's a trivial thing though...

@kzc

kzc commented Sep 28, 2017

Copy link
Copy Markdown
Contributor

@corneliusweig A combined PR would be helpful as it's a bit confusing. Thanks.

The individual commits need not be collapsed.

Looks at each expression in the sequence and only retains the node if it has an effect.

Note: in sequence expressions with effects always keep the last node, to be consistent when assigning the resulting value to a variable.
@corneliusweig
corneliusweig force-pushed the treeshake-sequence-expression branch from ecf1b89 to e5eb0ea Compare September 29, 2017 15:08
@corneliusweig

Copy link
Copy Markdown
Contributor Author

@kzc I've combined the PR #1650 with this one and collapsed the commits appropriately. It's still two commits: one for the work on SequenceExpression and one for ConditionalExpression.

@lukastaegert

Copy link
Copy Markdown
Member

Since this is not a breaking change and I really want this to be in the master before I make my next PR I will merge this now.

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.

Treeshaking of sequence expressions

3 participants