This repository has been archived by the owner on Oct 1, 2024. It is now read-only.
Support for async loading in Remix/Esbuild #2546
Closed
+17
−28
Conversation
This file contains 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
1. React no longer recommends this pattern and has removed the warning about "set state after unmounted may indicate a memory leak" because the intention was to help developers find places to cancel asynchronous actions but many can't be cancelled so this just leads to noise in the code. 2. This led to race conditions where a load() call is made while mounted is still false and the loaded result is never set in the value. The callback is never invoked a second time, so the value never gets set despite the load completing.
1. React no longer recommends this pattern and has removed the warning about "set state after unmounted may indicate a memory leak" because the intention was to help developers find places to cancel asynchronous actions but many can't be cancelled so this just leads to noise in the code. 2. This led to race conditions where a load() call is made while mounted is still false and the loaded result is never set in the value. The callback is never invoked a second time, so the value never gets set despite the load completing.
|
/snapit |
|
🫰✨ Thanks @ryanwilsonperkin! Your snapshots have been published to npm. Test the snapshots by updating your yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected]yarn add @shopify/[email protected] |
|
🔎 View this PR in Shipit Next. ℹ️ Expand to learn how to deploy and handle emergencies using Shipit NextOverviewShipit Next will merge your code on your behalf because this repository uses Shipit Next and its merge queue. To ship this PR, you can either:
Comment Commands
Commands exclusive to Deploy Before Merge
Documentation
Questions or feedback?
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Works together with #2538
When attempting to lazy load React components and GraphQL documents using Remix/Esbuild, we've found that the
loadcallback from the async resolver will often be fired while theuseMountedRefis returning false. This might indicate an edge case inuseMountedRef, potentially because of the use of ESM in the browser, or rendering under Strict Mode.This meant that the callback would be triggered and would resolve the async value, but because the mounted ref was set to false, we would not call the setState and so would never show the loaded value. By removing the mounted check, we get the value setting correctly.
We also believe that this is the right path forward regardless of this loading bug, because of a change introduced in React 18 to no longer warn in this scenario. Setting state when the component was unmounted would previously emit a warning, with the intent to get developers to cancel their async actions as a cleanup state when unmounting a component. Unfortunately, many async actions don't have an easy option for cancelling so it led many devs to create variants of
isMounted