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: use ResizeObserver global from parentNode realm to support case with multiple realms #82

Merged

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Feb 21, 2024

I am working on something for React DevTools, which requires us to start using ResizeObserver. After bumping this package to version 1.0.22, the onResize callback has stopped being executed in valid cases for the browser extension.

After some debugging, I've noticed that the parentNode.ownerDocument is different from document, which means that the code runs in a different realm from where parentNode was defined.

By running

console.log(this._autoSizer.parentNode.ownerDocument.defaultView.ResizeObserver, ResizeObserver, this._autoSizer.parentNode.ownerDocument.defaultView.ResizeObserver === ResizeObserver);
console.log(document, this._autoSizer.parentNode.ownerDocument, document === this._autoSizer.parentNode.ownerDocument);

I've checked document and ResizeObserver instances, this is the result:
Screenshot 2024-02-21 at 12 11 45

Essentially, this change is only about switching ResizeObserver to parentNode.ownerDocument.defaultView.ResizeObserver.

Happy to provide more context if needed, tried adding a test for it with some iframes, but this will probably take much more time than the change itself.

Copy link
Owner

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I'm pretty suck today so my brain isn't working so well.

Will this change still work with server rendering? I see you moved things around a bit, but can you confirm that you've tested to make sure it didn't regress?

@hoxyq
Copy link
Contributor Author

hoxyq commented Feb 21, 2024

Will this change still work with server rendering?

This logic happens in componentDidMount. AFAIK, this should not be called with server rendering.

I see you moved things around a bit, but can you confirm that you've tested to make sure it didn't regress?

Yeah, can confirm that:

  1. Ran pnpm test
  2. Ran pnpm prerelease to build a package and tested it with React DevTools.

@bvaughn
Copy link
Owner

bvaughn commented Feb 21, 2024

This logic happens in componentDidMount. AFAIK, this should not be called with server rendering.

That's fair. (The issue the comment referred to is bvaughn/react-virtualized#41 FWIW)

@bvaughn bvaughn merged commit 12f19ab into bvaughn:master Feb 21, 2024
@bvaughn
Copy link
Owner

bvaughn commented Feb 21, 2024

react-virtualized-auto-sizer@1.0.23

@hoxyq
Copy link
Contributor Author

hoxyq commented Feb 21, 2024

react-virtualized-auto-sizer@1.0.23

Thank you!

hoxyq added a commit to facebook/react that referenced this pull request Feb 22, 2024
…-auto-sizer to ^1.0.23 (#28408)

1. Bumps `react-virtualized-auto-sizer` to 1.0.23, which has a fix for
cases with multiple realms -
bvaughn/react-virtualized-auto-sizer#82
2. Removes `react-window` from react-devtools-shared/src/node_modules,
now listed as dependency in `package.json` and bumped to 1.8.10

Tested:
- Chrome extension
- Standalone shell with RN
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…-auto-sizer to ^1.0.23 (facebook#28408)

1. Bumps `react-virtualized-auto-sizer` to 1.0.23, which has a fix for
cases with multiple realms -
bvaughn/react-virtualized-auto-sizer#82
2. Removes `react-window` from react-devtools-shared/src/node_modules,
now listed as dependency in `package.json` and bumped to 1.8.10

Tested:
- Chrome extension
- Standalone shell with RN
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.

2 participants