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

fix: Ensure CSS support is checked more robustly #1106

Merged

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Feb 3, 2023

Noticed this while debugging tests in sentry-javascript. it seems jsdom has a weird support for CSSMediaRule, where it is defined but if you look at CSSMediaRule.prototype it is:

CSSRule {
        parentRule: null,
        parentStyleSheet: null,
        constructor: [Function: CSSMediaRule],
        type: 4
      }

I guess this could also be the case for other "weird" browsers or engines. I think it's safer to make sure everything we need is actually there at this point.

ref getsentry#42

@Juice10
Copy link
Contributor

Juice10 commented Feb 3, 2023

@mydea Thanks for submitting this PR, I had no idea rrweb was being used to record virtual pages with JSDOM, very interesting use case!
I added a small suggestion, if you go ahead and merge that suggestion, I'd be happy to approve this PR and get it merged.

Co-authored-by: Justin Halsall <Juice10@users.noreply.github.com>
@mydea
Copy link
Contributor Author

mydea commented Feb 3, 2023

@mydea Thanks for submitting this PR, I had no idea rrweb was being used to record virtual pages with JSDOM, very interesting use case! I added a small suggestion, if you go ahead and merge that suggestion, I'd be happy to approve this PR and get it merged.

Yeah, I only really found it due to tests using jsdom. But it showed that apparently some older CSS spec had this different, which may also happen in other browsers I guess - see: NV/CSSOM#105

I commited your suggestion 👍 thanks!

@mydea mydea closed this Feb 3, 2023
@mydea mydea deleted the fn/check-css-media-support-reliably branch February 3, 2023 13:36
@mydea mydea restored the fn/check-css-media-support-reliably branch February 3, 2023 13:38
@mydea mydea reopened this Feb 3, 2023
@mydea
Copy link
Contributor Author

mydea commented Feb 3, 2023

Oops sorry accidentally deleted this branch 😬 restored it and reopened the PR!

@YunFeng0817
Copy link
Member

YunFeng0817 commented Feb 8, 2023

@Juice10 Is it OK to merge this one?

@mydea
Copy link
Contributor Author

mydea commented Feb 8, 2023

OK, actually we ran into a problem with this (see getsentry#47), so I updated this PR to ensure we do not access window in module scope.

Now, this is only checked on-demand. I also split this into to methods, one to check when we only want to access this, and one when we want to monkey patch this.

@Juice10 Juice10 merged commit cb15800 into rrweb-io:master Feb 9, 2023
@mydea mydea deleted the fn/check-css-media-support-reliably branch February 9, 2023 08:21
@Juice10
Copy link
Contributor

Juice10 commented Feb 9, 2023

@mydea thanks for the refactor and the bugfix, I’ve merged the PR

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