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: Add missing React 18 reconciler function #206

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

banderson
Copy link
Contributor

Context

This adds a missing detachDeletedInstance function to @remote-ui/react/reconciler, which is invoked during React's clean up phase. Prior to this change you'd get an exception / broken app when a component is removed (like the example in the unit test of toggling an image component).

I created a minimal repro case on codesandbox and this is what that looks like in action:

Kapture 2023-02-10 at 23 57 46

Other notes

  • That function was added to react-reconciler in PR #21039
  • It is only intended to be called for "Host Components" / native DOM elements, but from tracing the behavior at runtime I noticed that all createRemoteReactComponent-generated components seem to e classified as Host Components by the reconciler
  • This behavior only occurs when removing previously-rendered components
  • Full exception stack trace:
Uncaught TypeError: detachDeletedInstance is not a function
    at detachFiberAfterEffects (react-reconciler.development.js:15352:9)
    at detachFiberAfterEffects (react-reconciler.development.js:15329:5)
    at detachFiberAfterEffects (react-reconciler.development.js:15329:5)
    at commitPassiveUnmountEffectsInsideOfDeletedTree_complete (react-reconciler.development.js:16750:7)
    at commitPassiveUnmountEffectsInsideOfDeletedTree_begin (react-reconciler.development.js:16735:7)
    at commitPassiveUnmountEffects_begin (react-reconciler.development.js:16634:11)
    at commitPassiveUnmountEffects (react-reconciler.development.js:16619:3)
    at flushPassiveEffectsImpl (react-reconciler.development.js:19181:3)
    at flushPassiveEffects (react-reconciler.development.js:19127:14)
    at react-reconciler.development.js:18912:9

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! I changed the bump to a patch since this shouldn't cause anyone any issues.

@lemonmade lemonmade merged commit f95d0c6 into Shopify:main Feb 15, 2023
This was referenced Feb 15, 2023
@lemonmade
Copy link
Member

Released in @remote-ui/react@5.0.1

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