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

Treeshake sequence expression #1650

Merged

Conversation

corneliusweig
Copy link
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
Copy link
Contributor

kzc commented Sep 27, 2017

Is #1648 a superset of this PR?

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.

I really like this! No objections to merging this.

@lukastaegert
Copy link
Member

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

@corneliusweig
Copy link
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
Copy link
Contributor

kzc commented Sep 28, 2017

@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
Copy link
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
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