Resolve #2191: Add spaces after return and yield if necessary - #2194
Conversation
guybedford
left a comment
There was a problem hiding this comment.
Looks good. If other cases crop up we can handle them similarly too.
| this.argument.render(code, options); | ||
| if (this.argument.start === this.start + 5 /* 'yield'.length */) { | ||
| code.prependLeft(this.start + 5, ' '); | ||
| } |
There was a problem hiding this comment.
Should we try and preserve handling of return!x style cases though here?
|
Are any operators affected by this expression simplification spacing issue as well?
|
|
Operator precedence says it is not necessary. This situation can only occur if to the right side of the operator, there is an expression that can be simplified. The only expressions (for now) that can be simplified are logical, conditional and sequence expressions. All of those bind weaker than the operators you listed. But yes, this issue could crop up again in the future if we extend value inlining/simplification enough. |
|
@guybedford Thanks for the quick reviews on these ones! |
Are parens ever dropped for simplified expressions? If so, that could be an issue. |
|
Released as 0.59.1 |
If the parent contains parens, those will always be retained. In some situations, this can lead to double parens but this is usually not an issue. |
This Pull Request updates dependency [rollup](https://github.com/rollup/rollup) from `v0.59.0` to `v0.59.4` <details> <summary>Release Notes</summary> ### [`v0.59.4`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0594) [Compare Source](rollup/rollup@v0.59.3...v0.59.4) *2018-05-28* * Fix performance regression when many return statements are used ([#​2218](`https://github.com/rollup/rollup/pull/2218`)) --- ### [`v0.59.3`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0593) [Compare Source](rollup/rollup@v0.59.2...v0.59.3) *2018-05-24* * Fix reassignment tracking for constructor parameters ([#​2214](`https://github.com/rollup/rollup/pull/2214`)) --- ### [`v0.59.2`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0592) [Compare Source](rollup/rollup@v0.59.1...v0.59.2) *2018-05-21* * Fix reassignment tracking in for-in loops ([#​2205](`https://github.com/rollup/rollup/pull/2205`)) --- ### [`v0.59.1`](https://github.com/rollup/rollup/blob/master/CHANGELOG.md#​0591) [Compare Source](rollup/rollup@v0.59.0...v0.59.1) *2018-05-16* * Fix infinite recursion when determining literal values of circular structures ([#​2193](`https://github.com/rollup/rollup/pull/2193`)) * Fix invalid code when simplifying expressions without spaces ([#​2194](`https://github.com/rollup/rollup/pull/2194`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
After some deliberation, this is the lowest impact fix I could come up with to resolve #2191. Basically as far as I am aware, this issue can only arise for
returnstatements andyieldexpressions when there is no space between the keyword and the subsequent expression and the nested expression changes due to some simplification.E.g.
return!1||true->return true.For
awaitexpressions this is not an issue as operator precedence is different here (await!1||trueis equivalent to(await !1) || true)The solution is to insert a space if the argument of the
returnstatement starts directly after the last letter ofreturn, similarly foryield.