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

[WIP] hack together stylesheet observer #124

Closed
wants to merge 1 commit into from

Conversation

jperl
Copy link
Contributor

@jperl jperl commented Aug 25, 2019

This is my very hacky fix implementing #104

@marcospassos
Copy link
Contributor

marcospassos commented Aug 25, 2019

Hi @jperl, thank you for working on this.

One problem I see with your suggestion is the lack of support of Proxy. While not sure if this is the best way to tackle the problem, I would recommend using monkey-patching instead of a proxy. Even not ideal, it ensures backward compatibility.

@jperl
Copy link
Contributor Author

jperl commented Aug 25, 2019

@marcospassos Good point I just copied in this code without thinking about it -- that is an easy change to make.

Where do you recommend I inject the monkey patch -- for both the initial load and newly created elements?

Also how do you recommend testing this?

@marcospassos
Copy link
Contributor

marcospassos commented Aug 25, 2019

@jperl, I'd take another path for solving this problem:

  • Monkey-patch the insertRule and removeRule globally, so that we don't need to track recently changed or newly added elements.
  • Create a mechanism to stop listening to changes as soon as the recorder is turned off
  • Create a new incremental snapshot event to explicitly report a CSS rule addition or removal
  • Change the replayer to apply those changes to the document

Benefits

  • Better performance, as we don't have to watch elements being added or changed
  • More granularity in the event reporting (we'll only track the changes rather than the entire stylesheet)
  • Improved browser support
// How to monkey-patch the insertRule globally
const insertRule = CSSStyleSheet.prototype.insertRule;

CSSStyleSheet.prototype.insertRule = function (rule: string) {
    dispatchEvent(/*...*/);

    return insertRule.apply(this, arguments);
}

@jperl
Copy link
Contributor Author

jperl commented Aug 25, 2019

@marcospassos That sounds like a much better approach.

@Yuyz0112 if you can confirm that you would be willing to merge the approach @marcospassos recommended, I am happy to take a stab at implementing it.

@Yuyz0112
Copy link
Member

@marcospassos Thanks for the review!
@jperl I agree with the suggestions from @marcospassos and feel free to ping me if you meet any issue during implement this.

If you want to split this implementation into multiple PRs(which is recommended), I think it can be done with:

  1. patch insertRule/deleteRule APIs.
  2. add incremental events and observers in recording side.
  3. replay the stylesheet events in the replaying side.

@jperl
Copy link
Contributor Author

jperl commented Aug 26, 2019

Sounds good I will split it into multiple PRs. EDIT: tracking in my sprint board here https://github.com/qawolf/qawolf/issues/121

@dcramer
Copy link
Contributor

dcramer commented Feb 7, 2020

@jperl I might take a stab at moving forward with your work unless you were still actively looking into this. Let me know!

@jperl
Copy link
Contributor Author

jperl commented Feb 7, 2020

@dcramer Please do! Sorry I have been too busy to tackle this yet.

@dcramer
Copy link
Contributor

dcramer commented Feb 21, 2020

I'm working on adapting this patch, and hoping someone can help clarify something.

I've disabled the observers and written a test which uses insertRule/deleteRule, however the test passes successfully where I'd expect it to fail. I believe thats because of the existing element mutation observers. How do I actually reproduce the core issue in isolation?

@dcramer
Copy link
Contributor

dcramer commented Feb 21, 2020

NEVERMIND

@Yuyz0112
Copy link
Member

Looks like we can close this one since #177 has been merged?

@jperl @dcramer

@dcramer
Copy link
Contributor

dcramer commented Feb 22, 2020

👍 i just need to figure out how to get things into the replayer but ill work on that next opportunity i get

p-mazhnik pushed a commit to p-mazhnik/rrweb that referenced this pull request Mar 12, 2024
We are always passing in fresh objects to this method, so instead of
cloning this into a new object, we can just put the `timestamp` on the
given object directly and return it, saving a bit of processing cost.
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.

5 participants