Skip to content

Fix regressions - #1586

Merged
Rich-Harris merged 5 commits into
rollup:masterfrom
lukastaegert:fix-regressions
Aug 28, 2017
Merged

Fix regressions#1586
Rich-Harris merged 5 commits into
rollup:masterfrom
lukastaegert:fix-regressions

Conversation

@lukastaegert

@lukastaegert lukastaegert commented Aug 28, 2017

Copy link
Copy Markdown
Member

This resolves #1584 and #1585 by introducing a new options parameter object describing the current execution path to Node.hasEffects() and similar functions.
For now, there are two options:

  • inNestedFunctionCall = false/true tells us if we are looking at effects of statements inside a nested function call. If we are inside a function, then all return statements should be kept; however, they should not count as effects for nested function calls
  • inNestedBreakableStatement = false/true tells us if we are looking at effects inside a nested loop or switch statement (i.e. something that can contain BreakStatements). If we are inside e.g. a switch statement, then break statements should never be omitted; however, they should not count as effects for nested switch statements/loops

It should also test for and fix #1587.

* Add more nodes
* Fix dropped declarations in loops where the loop variable is not used
* Fix side-effect detection for break-statements
@Conduitry

Copy link
Copy Markdown
Contributor

👍 Using this branch on the project where I was seeing #1584 and #1585, I'm no longer having those issues.

@swernerx

Copy link
Copy Markdown
Contributor

Wondering if this also has a positive effect on #1588

@lukastaegert

Copy link
Copy Markdown
Member Author

It seems that #1588 is actually the same as #1585, so I am quite confident.

@lukastaegert

Copy link
Copy Markdown
Member Author

Will resolve #1588

@lukastaegert

Copy link
Copy Markdown
Member Author

Until this is merged, you should be able to install rollup from this pull request via

"rollup": "rollup/rollup#pull/1586/head"

in your package.json

@Rich-Harris
Rich-Harris merged commit bc0745f into rollup:master Aug 28, 2017
@Rich-Harris

Copy link
Copy Markdown
Contributor

released as 0.49.1 — thank you!

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.

this.declaration.assignExpression is not a function await without function call is dropped

4 participants