rewrite: stop prepending semicolon to `this.` special property access…

ato commented 3 months ago

Fixes #850

Description

This changes removes the regex rewrite rule that prepends a semicolon when this. special property access occurs immediately after a newline.

For example:

x = 1 + 2
this.location = "foo"

will now be rewritten as:

x = 1 + 2
_____WB$wombat$check$this$function_____(this).location = "foo"

instead of:

x = 1 + 2
;_____WB$wombat$check$this$function_____(this).location = "foo"

Motivation and Context

The prepended semicolon breaks code (such as jQuery in #850) that looks like:

foo = foo ? foo :
            this.location;

I think the reason we started inserting the semicolon was because in situations like:

x = 1 + 2
this.location = "foo"

we used to rewrite to:

x = 1 + 2
(this && this._WB_wombat_obj_proxy || this).location = "foo"

which the browser would interpret as a bogus function call like 2(this && ... ). But nowadays prepending the semicolon should be unnecessary as we currently rewrite to:

x = 2 + 3
_____WB$wombat$check$this$function_____(this).location = "foo"

which will trigger JavaScript's automatic semicolon insertion rules like the original code does.

Types of changes

[x] Replay fix (fixes a replay specific issue)
[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have added or updated tests to cover my changes.
[x] All new and existing tests passed.

tw4l commented 3 months ago

Hi @ato , now that we've worked out the dependency upgrades and related test issues in main, would you mind rebasing this on latest main? Thanks!

ato commented 3 months ago

Rebased. :-)

ikreymer commented 2 months ago

Thanks for this @ato (and remembering the history of the adding the semicolon).

In wabac.js, we made this a more involved check to see if it the previous character is actually a newline, and a semicolon only then: https://github.com/webrecorder/wabac.js/blob/main/src/rewrite/jsrewriter.js#L76 from matching: https://github.com/webrecorder/wabac.js/blob/main/src/rewrite/jsrewriter.js#L115 so we don't have that issue there, but maybe even that is unnecessary?

webrecorder / pywb