Skip to content

Fix for #2018 - #2019

Merged
lukastaegert merged 1 commit into
masterfrom
system-default-formatting
Mar 5, 2018
Merged

Fix for #2018#2019
lukastaegert merged 1 commit into
masterfrom
system-default-formatting

Conversation

@guybedford

Copy link
Copy Markdown
Contributor

Fixes the export default { obj } case being output as exports('default', { obj )} instead of exports('default', { obj }).

);
if (systemBinding) {
code.prependRight(this.end - 1, ')');
code.appendRight(code.original[this.end - 1] === '}' ? this.end : this.end - 1, ')');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is the proper fix. If in the entrypoint-facade test you change the default export to export default 42 without a semicolon, the system output becomes exports('default$1', 4)2. Instead I would suggest to check for the presence of a semicolon as the last character.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that works, and makes me feel a lot better about the fix as well understanding the issue!

@lukastaegert lukastaegert added this to the 0.56.4 milestone Feb 28, 2018
@guybedford
guybedford force-pushed the system-default-formatting branch from 16f28a0 to cd7bf17 Compare February 28, 2018 13:37
}
code.overwrite(expression.start, expression.end,
`(exports(${this.variable.exportName}, ${op}), ${name}${expression.operator})`);
`(exports('${this.variable.exportName}', ${op}), ${name}${expression.operator})`);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, but do you think we might reflect this in a test somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me finally get around to adding system format expectations for the "form" folder in general, which would catch all these cases. Will aim to do this as a separate PR though. Otherwise I'm just going to be continuing to pollute the chunking-form tests to try and cover everything further.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be awesome!

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.

2 participants