Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse monkey patch built in methods to support LWC #1509

Merged
merged 206 commits into from
Aug 2, 2024

Conversation

Juice10
Copy link
Contributor

@Juice10 Juice10 commented Jun 14, 2024

LWC monkey patches a lot of built in methods.

This PR gets around that issue by pulling new references from a temporary iframe, which hasn't been monkey patched yet.

Turns out what we where doing by overwriting `public textContent: string | undefined` as a getter in a subclass is something that isn't allowed in typescript. Because we where using `// @ts-ignore`  to hide this error our bundler chose to allow the overwrite. Vite choses to disallow the overwrite making all subclasses' `textContent` undefined.
To mitigate this we're using an abstract class, which does allow sub classes to decide if they wan't to use getters or not.
This removes console from rrweb-all.js
something when wrong earlier when resolving merge conflicts, this should be correct
This feels incorrect to me (Justin Halsall), but some of the tests break without it so I'm restoring this to be closer to its original here:
https://github.com/rrweb-io/rrweb/blame/cfd686d488a9b88dba6b6f8880b5e4375dd8062c/packages/rrweb-snapshot/src/snapshot.ts#L1011
@eoghanmurray
Copy link
Contributor

I've asked a SO question here to see if there are any ideas on how we might achieve the goals of this PR with less invasive code changes:
https://stackoverflow.com/questions/78633141/typescript-transform-property-to-function-call

Could we add a new (optional) output target, which pulls in the added functions, and also includes a rollup plugin to find/replace all the needed property accesses?

https://www.npmjs.com/package/rollup-plugin-modify#example-using-regexp-for-find-and-a-function-for-replace

@eoghanmurray
Copy link
Contributor

eoghanmurray commented Jun 17, 2024

From SO, the first replier mentions https://www.npmjs.com/package/ts-patch
I haven't looked at it, but maybe would work better than a regex solution

@Juice10 Juice10 marked this pull request as ready for review June 19, 2024 09:01
@Juice10 Juice10 changed the title [draft] reverse monkey patch built in methods to support LWC Reverse monkey patch built in methods to support LWC Jun 24, 2024
@Juice10
Copy link
Contributor Author

Juice10 commented Jun 24, 2024

I've asked a SO question here to see if there are any ideas on how we might achieve the goals of this PR with less invasive code changes: https://stackoverflow.com/questions/78633141/typescript-transform-property-to-function-call

Could we add a new (optional) output target, which pulls in the added functions, and also includes a rollup plugin to find/replace all the needed property accesses?

https://www.npmjs.com/package/rollup-plugin-modify#example-using-regexp-for-find-and-a-function-for-replace

I agree that it would be really nice if we could use some sort of general purpose solution to rewrite this out of the box. Currently we have a couple things that make this difficult:

  • we only need the rewrite on record side, not replay, and only some shared utils
    • if we move all record code to @rrweb/record we could possibly do this
  • one record side we can't rewrite everything because for example we have serializedNode's which include a childNodes reference, and those should not go via the rewrite
    • to deal with this one could add extra checks to see if the node that was passed to childNodes(NODE) is actually a dom node.
      • Downside: instanceof Node won't work as it has to support iframes, so checks become more complicated
      • Downside: every extra check we add is more performance overhead since many of these functions are already hot-paths themselves.
    • we can't gracefully send the wrong type of node to childNodes(NODE) as the errors are pretty harsh and unforgiving.
  • tests need to happen using the rewritten code
    • otherwise we might break something and only find out in production
    • we could do this by moving all record tests to @rrweb/record, have it for patched on build time for @rrweb/record. And we'd need to move all integration/e2e tests to @rrweb/all, but which should use the patched version of @rrweb/record under the hood.

@YunFeng0817 YunFeng0817 self-requested a review July 5, 2024 08:27
@eoghanmurray
Copy link
Contributor

eoghanmurray commented Jul 9, 2024

I've just noticed that I've added code in stylesheet-assets branch that runs over n.childNodes but can accept either a serializedElementNodeWithId or a HTMLStyleElement (or an rrdom equivalent):
https://github.com/rrweb-io/rrweb/pull/1475/files#diff-0d9a03e75eacff8f6be89336b49419b210455c1a81d8f46fae7cea2d83a8f93eR156

Presumably I'll have to do some type checking to figure out whether to call n.childNodes or dom.childNodes(n) etc. :(

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

Great feature for recording LWS based websites!

@Juice10 Juice10 merged commit be6bf52 into master Aug 2, 2024
11 checks passed
@Juice10 Juice10 deleted the juice10/reverse-monkey-patch branch August 2, 2024 08:55
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.

3 participants