Skip to content

fix: Fix checking for patchTarget in initAdoptedStyleSheetObserver#1327

Merged
YunFeng0817 merged 3 commits intorrweb-io:masterfrom
getsentry:fn/add-stylesheet-guard
Oct 13, 2023
Merged

fix: Fix checking for patchTarget in initAdoptedStyleSheetObserver#1327
YunFeng0817 merged 3 commits intorrweb-io:masterfrom
getsentry:fn/add-stylesheet-guard

Conversation

@mydea
Copy link
Contributor

@mydea mydea commented Oct 13, 2023

Calling Object.getOwnPropertyDescriptor(undefined, 'xx') actually errors out, so let's guard there. The types for this are bad, because it takes any but actually fails on nullish objects 😬

@mydea
fix: Fix checking for patchTarget in initAdoptedStyleSheetObserver
b4af692 
Calling `Object.getOwnPropertyDescriptor(undefined, 'xx')` actually errors out, so let's guard there.
@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 3f6e65e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mydea mydea mentioned this pull request Oct 13, 2023
Merged
@mydea
Copy link
Contributor Author

mydea commented Oct 13, 2023

Hmm, when I run yarn changeset I get this error:

yarn changeset
yarn run v1.23.0-20220130.1630
$ ~/rrweb/node_modules/.bin/changeset
🦋  error ValidationError: Some errors occurred when validating the changesets config:
🦋  error The package or glob expression "rrweb" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "rrweb-snapshot" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "rrdom" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "rrdom-nodejs" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "rrweb-player" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "@rrweb/types" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "@rrweb/web-extension" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.
🦋  error The package or glob expression "rrvideo" specified in the `fixed` option does not match any package in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@mydea
Copy link
Contributor Author

mydea commented Oct 13, 2023

Hmm not sure if the test failure is really related to this PR? 🤔 can't see how this should lead to failing tests 😅

@YunFeng0817
Copy link
Member

YunFeng0817 commented Oct 13, 2023

@mydea This pull request is ready to go once you add the change set log

@eoghanmurray
Copy link
Contributor

eoghanmurray commented Oct 13, 2023

I've re-run the tests just now

Could you describe what was triggering the error? Presence of a 3rd party library? Or a particular browser version?

Can you click on the "Click here if you're a maintainer who wants to add a changeset to this PR" link? Maybe you don't have permissions for that?

@mydea
Copy link
Contributor Author

mydea commented Oct 13, 2023

Can you click on the "Click here if you're a maintainer who wants to add a changeset to this PR" link? Maybe you don't have permissions for that?

Ah, that button worked!

(Side note, is yarn changeset supposed to work? Or should you do something else to generate a changeset locally?)

Could you describe what was triggering the error? Presence of a 3rd party library? Or a particular browser version?

We've had some users come up that they got errors captured from that line with exactly the error you'd expect if patchTarget was undefined. Not 100% sure how they get to this state, but alone from a type safety perspective it makes sense (*apart from the fact that Object. getOwnPropertyDescriptor is terribly typed)

@YunFeng0817 YunFeng0817 merged commit 57a940a into rrweb-io:master Oct 13, 2023
@mydea mydea deleted the fn/add-stylesheet-guard branch October 13, 2023 12:01
billyvg pushed a commit to getsentry/rrweb that referenced this pull request Oct 20, 2023
@mydea @billyvg
fix: Fix checking for patchTarget in initAdoptedStyleSheetObserver (
90537c0 
rrweb-io#1327)
@pauldambra pauldambra mentioned this pull request Nov 16, 2023
Closed
1 task
This was referenced Dec 4, 2023
Merged
Open
Closed
@github-actions github-actions bot mentioned this pull request Feb 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YunFeng0817 YunFeng0817 YunFeng0817 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mydea @YunFeng0817 @eoghanmurray