Fix placement of inserted semicolon when using SystemJS

[kyle1320]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
#2527

Description

Fixes #2527.

Currently, semicolons are inserted using appendLeft. But this means that any code inserted after the end of the line will be inserted after the semicolon, which is not what we want.

This fixes the issue by using appendRight instead.

[lukastaegert]

Hi @kyle1320,

thanks a lot, you are really becoming quite the contributor here 😎
Very nice test, and the solution is definitely valid at the moment. My feeling is, though, that we should fix it in another location, but let me explain first.

The append/prependLeft/Right methods actually work the following way:
Left and Right are important for tree-shaking. If something is added Left, then it is removed when the code to the left is removed. Similarly for Right. append and prepend then determine the insertion order:

• prepend will put the code in front of everything that has been added to this location before, even other prepended code
• append will put it after all added (appended or prepended) code

So my worries are about tree-shaking. Using appendRight will associate the semi-colon not with the actual expression statement but with the white-space following it. At the moment this is not an issue because the white-space until the next line-break will be removed together with the statement but this is not guaranteed to be stable in the future.

Instead I would prefer to fix it in AssignmentExpression.ts. Instead of

code.prependRight(this.right.end, `)`);

which is definitely looks wrong following my above explanation, the code should probably be

code.appendLeft(this.right.end, `)`);

[kyle1320]

@lukastaegert Thanks for the feedback.

That actually makes a lot of sense! This was my first experience with magic-string, so I was mostly just going off of the documentation. That's why we have reviews I suppose 🙂

I thought I had tried changing the statement in AssignmentExpression.ts before, but after trying it again (and understanding it better now), it works!

I've pushed an update and changed the title of the PR to better reflect the scope of the change.

[lukastaegert]

Looks good, thanks a lot!