Have rollup emit `let $safeClassName = class $originalClassName { ...… - #2025
Conversation
| if (this.id) { | ||
| const name = this.id.variable.getName() | ||
| if (name !== this.id.variable.name) { | ||
| code.appendRight(this.start, `let ${this.id.variable.safeName} = `) |
There was a problem hiding this comment.
Not sure appendRight is the correct one. prependLeft doesn't work, in any case.
There was a problem hiding this comment.
Yes, appendRight is actually the right one. The name may feel a little counter-intuitive but it means
- insert at the current position (just like
appendLeft,prependRightandprependLeft) - if anything else was inserted at this position earlier, put this insertion last (this is the
appendpart and certainly the right choice as we would not want anything to slip in between this insertion and the class) - if the code to the right is removed later for some other reason, remove this insertion as well (this is the
rightpart and the reason whyleftwould definitely be wrong here). Sorightmeans "attach this to the subsequent code" whileleftmeans "attach this to the preceding code"
|
Fixes the class part of #1914 |
f8dc2a8 to
2dc6c7d
Compare
|
Nice thanks for the PR! It seems like this is missing the test expectations from the commit? |
|
As far as I could tell, the test/function tests are only checked by running the generated bundle? Where would expected baselines go? |
|
Ah I misread this as a form test not a function one. |
guybedford
left a comment
There was a problem hiding this comment.
Seems good to me. I'd be happier seeing output tests to ensure this works with all the formats, but thinking through the cases it seems like it will.
2dc6c7d to
2c64d83
Compare
|
@guybedford Added a form test. |
|
|
||
| let MyClass$1 = class MyClass { | ||
| constructor() { } | ||
| } |
There was a problem hiding this comment.
Actually I think this is incorrect if we don't have automatic semicolon insertion here.
For example:
class Q {
} p();is valid JavaScript, while:
let Q$1 = class Q {
} p()is not!
There was a problem hiding this comment.
Good catch. Should I just unconditionally add a semicolon (or maybe a new line)? From what I could tell, it's not possible to check whether the next token is preceded by a new line without doing additional parsing manually.
There was a problem hiding this comment.
See https://github.com/rollup/rollup/blob/master/src/ast/nodes/ExportDefaultDeclaration.ts#L112 for a rough example of how this can work. I think the crux is just:
if (code.original[this.end] !== ';') {
code.prependRight(this.end, ';');
}There was a problem hiding this comment.
Ah, thanks for the example.
|
@guybedford I changed it to always add a semicolon: class declarations don't require semicolons, so if you have It might be worth checking if that's also an issue in the example you linked... |
guybedford
left a comment
There was a problem hiding this comment.
Looks good, boundary handling sounds fine from what you say. Thanks for the quick follow-up!
lukastaegert
left a comment
There was a problem hiding this comment.
Just a slight change with the use of left vs right, otherwise this is good to go!
| if (this.id) { | ||
| const name = this.id.variable.getName() | ||
| if (name !== this.id.variable.name) { | ||
| code.appendRight(this.start, `let ${this.id.variable.safeName} = `) |
There was a problem hiding this comment.
Yes, appendRight is actually the right one. The name may feel a little counter-intuitive but it means
- insert at the current position (just like
appendLeft,prependRightandprependLeft) - if anything else was inserted at this position earlier, put this insertion last (this is the
appendpart and certainly the right choice as we would not want anything to slip in between this insertion and the class) - if the code to the right is removed later for some other reason, remove this insertion as well (this is the
rightpart and the reason whyleftwould definitely be wrong here). Sorightmeans "attach this to the subsequent code" whileleftmeans "attach this to the preceding code"
| const name = this.id.variable.getName() | ||
| if (name !== this.id.variable.name) { | ||
| code.appendRight(this.start, `let ${this.id.variable.safeName} = `) | ||
| code.prependRight(this.end, ';'); |
There was a problem hiding this comment.
This, on the other hand, needs to be prependLeft with the explanation given above.
…` instead of
`class $safeClassName { ...`, which changes the runtime behavior when MyClass.name
is accessed.
Added corresponding tests.
8685092 to
785fdfc
Compare
|
@lukastaegert done, and I rebased it. |
…` instead of
class $safeClassName { ..., which changes the runtime behavior when MyClass.nameis accessed.
Added corresponding test.