System format tests - #2022
Conversation
lukastaegert
left a comment
There was a problem hiding this comment.
Nice to finally have this! Just some minor nits about the magic-string usage, otherwise this is ok from my side.
| const used = blank(); | ||
| const es = options.format === 'es'; | ||
| const system = options.format === 'system'; | ||
| const es = options.format === 'es' || options.format === 'system'; |
| if (options.systemBindings && this.id.variable.exportName) { | ||
| code.appendRight(this.end, ` exports('${this.id.variable.exportName}', ${this.id.variable.getName()});`); | ||
| if (options.systemBindings && this.id && this.id.variable.exportName) { | ||
| code.overwrite(this.end - 1, this.end, `} exports('${this.id.variable.exportName}', ${this.id.variable.getName()});`); |
There was a problem hiding this comment.
I understand this fix and it works because class declarations always end with a "}" but conceptually, I would stay with the original approach and use appendLeft instead of appendRight. appendRight code is removed if the code following the insertion is removed while appendLeft is removed if the preceding code is removed. Since this code only ever makes sense if the last character of the preceding class declaration is retained, appendLeft (or maybe prependLeft, not sure if there is ever a situation where this would make a difference here) seems to me to be the better and maybe even safer choice.
There was a problem hiding this comment.
Nice, that works much better!
| if (options.systemBindings && isClassDeclaration(this.declaration)) { | ||
| code.appendRight(this.end, ` exports('default', ${name});`); | ||
| if (options.systemBindings && isClassDeclaration(this.declaration) && this.variable.exportName) { | ||
| code.appendRight(this.end, ` exports('${this.variable.exportName}', ${name});`); |
There was a problem hiding this comment.
As in ClassDeclaration, I guess this should be appendLeft.
This fleshes out the System module format tests for the whole "form" folder, and caught a few bugs in the process which have been corrected here too, specifically: