Expose instance handles from shadow nodes directly, instead of from event emitters/targets #37553
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
1 similar comment
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 24, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Differential Revision: D46149084 fbshipit-source-id: 0871a15b16b3abb7ab6d695cce404d6d5598eea4
rubennorte
force-pushed
the
export-D46149084
branch
from
bea4d1b to
ed48b04
Compare
Base commit: 108309e |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
force-pushed
the
export-D46149084
branch
from
ed48b04 to
7749ade
Compare
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 25, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Differential Revision: D46149084 fbshipit-source-id: 703b2d6f9d3f9662cb83f2a7de5aea71fe7ca989
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 25, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 62812beaff1160114bd3c401ca92833b3abfa8b0
rubennorte
force-pushed
the
export-D46149084
branch
from
7749ade to
82b623a
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 26, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 3b00f4490ab26f557e99a314e704c3628095860a
rubennorte
force-pushed
the
export-D46149084
branch
from
82b623a to
21bfce5
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 26, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 12acb1b31c74174afe9506335307b2bfd59a918e
rubennorte
force-pushed
the
export-D46149084
branch
from
21bfce5 to
76d31a2
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 26, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: e0af3f829fbd545d000abf910c21c465bc16df2a
rubennorte
force-pushed
the
export-D46149084
branch
from
76d31a2 to
523f18b
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 5, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 619d914485e6c99893091f9c6695606cf3a7a9e2
rubennorte
force-pushed
the
export-D46149084
branch
from
523f18b to
a63151d
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 5, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 2f21e5e4327309a81ed3f7eae853d40187f13889
rubennorte
force-pushed
the
export-D46149084
branch
from
a63151d to
595bd6a
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 6, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: d39152061efa9d82bf63099f36cdeeb988f7d037
rubennorte
force-pushed
the
export-D46149084
branch
from
595bd6a to
3142bbb
Compare
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 6, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 91faf5dd5f5c1c349ead74e046a459f8ca8fd564
rubennorte
force-pushed
the
export-D46149084
branch
from
3142bbb to
6ceff80
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 8, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: e1cfbfe69fba033cb3b11c08aa14164d2a53614a
rubennorte
force-pushed
the
export-D46149084
branch
from
6ceff80 to
ee37a7d
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 8, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: aba3478b2c7751edf916b027014f0207daa934e7
rubennorte
force-pushed
the
export-D46149084
branch
from
ee37a7d to
7529159
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
Jun 8, 2023
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 97abed87fa87984c14a4d9683801b76e5fdd6c68
rubennorte
force-pushed
the
export-D46149084
branch
from
7529159 to
51d5e49
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
…w node wrappers Differential Revision: D46190383 fbshipit-source-id: 06e0354352d0f42559f4c899dbc8d8a27dfe108f
… new instances of ShadowNodeFamily Differential Revision: D46190382 fbshipit-source-id: 485168423c3e60f94467e7b4b1c97182b4230e57
…vent emitters/targets (facebook#37553) Summary: Pull Request resolved: facebook#37553 ## Context I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in facebook@43864a1 (D44022477), so I could implement DOM traversal methods easily. I wanted to reuse that method to implement `MutationObserver` and `IntersectionObserver`, but unfortunately `EventEmitter`/`EventTarget` only allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as with `MutationObserver` we intentionally want to have access to unmounted nodes when creating the list of `addedNodes` and `removedNodes` for the `MutationRecord` (depending on when exactly we generate this list, either `addedNodes` or `removedNodes` wouldn't be mounted). ## Changes This reverts my original change and adds a new field in `ShadowNodeFamily` to provide access to the `InstanceHandle` at any point (even if the node isn't mounted). This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks. Changelog: [internal] Reviewed By: sammy-SC Differential Revision: D46149084 fbshipit-source-id: 7bb5be39a28188794d016befc9d27c178d67fd6c
rubennorte
force-pushed
the
export-D46149084
branch
from
51d5e49 to
b30c81b
Compare
Contributor
|
This pull request was exported from Phabricator. Differential Revision: D46149084 |
Contributor
|
This pull request has been merged in 3250725. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Context
I added some getters in EventTarget/EventEmitter to access the React instance handle for a given shadow node/shadow node family in 43864a1 (D44022477), so I could implement DOM traversal methods easily.
I wanted to reuse that method to implement
MutationObserverandIntersectionObserver, but unfortunatelyEventEmitter/EventTargetonly allow access to the instance handle as long as the shadow node is mounted. That makes sense for events, but not for this use case, as withMutationObserverwe intentionally want to have access to unmounted nodes when creating the list ofaddedNodesandremovedNodesfor theMutationRecord(depending on when exactly we generate this list, eitheraddedNodesorremovedNodeswouldn't be mounted).Changes
This reverts my original change and adds a new field in
ShadowNodeFamilyto provide access to theInstanceHandleat any point (even if the node isn't mounted).This provides the same guarantees as the original method, keeping weak references to the handles to avoid memory leaks.
Changelog: [internal]
Differential Revision: D46149084