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: Fix checking for patchTarget in initAdoptedStyleSheetObserver #1327

Merged
merged 3 commits into from
Oct 13, 2023

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 😬

Calling `Object.getOwnPropertyDescriptor(undefined, 'xx')` actually errors out, so let's guard there.
@changeset-bot
Copy link

changeset-bot bot commented Oct 13, 2023

🦋 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
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

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

@eoghanmurray
Copy link
Contributor

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)

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