[crud] Basic implementation #31523
Open
[crud] Basic implementation #31523
+1,260
−29
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 380f5d6...40630e4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
This PR introduces a new experimental hook `useResourceEffect`, which is something that we're doing some very early initial tests on. This may likely not pan out and will be removed or modified if so. Please do not rely on it as it will break.
jackpope
reviewed
Nov 14, 2024
| @@ -146,19 +153,56 @@ export function commitHookEffectListMount( | |||
|
|
|||
| // Mount | |||
| let destroy; | |||
| if (enableUseResourceEffectHook) { | |||
| if (effect.kind === ResourceEffectIdentityKind) { | |||
| effect.resource = effect.create(); | |||
There was a problem hiding this comment.
We might need a __DEV__ branch similar to what we have in the SimpleEffectKind flow where we handle the stacks.
Something like
effect.resource = runWithFiberInDEV(
finishedWork,
callCreateInDEV,
effect,
);
Similar for the update calls. I see for destroy you've added safelyCallDestroyWithResource to handle it.
Comment on lines
+313
to
+314
| effect.next.resource = null; | ||
| effect.next.update = undefined; |
There was a problem hiding this comment.
Probably the resource and update should live on the effect's instance object as well since they are stateful and modified in the same way as the destroy function.
It still feels unsafe to reach into the next effect. They are always pushed together though.
For the sake of the experiment, how about at least providing an else branch here with an error
if (effect.next.kind === ResourceEffectUpdateKind) {
resetInstance(effect.next)
} else {
throw new Error('Found identify effect without an update effect. This is a bug in React.');
}
Also add this above when assigning resource on the next effect
| inst: EffectInstance, | ||
| create: () => mixed, | ||
| createDeps: Array<mixed> | void | null, | ||
| destroy: ((resource: mixed) => void) | void, |
There was a problem hiding this comment.
Mentioned above in regards to reseting the instances, but moving pushing the destroy function onto the instance (where it lives for SimpleEffect), as well as resource, makes these effect types more consistent. Then if the create/update and createDeps/updateDeps are made more generic, we're back to monomorphic shapes in the effect list.
| }); | ||
| }).toErrorDev( | ||
| 'useResourceEffect received a dependency array with no dependencies. ' + | ||
| 'When specified, the dependency array must have at least one dependency.', |
There was a problem hiding this comment.
At this point they have to be specified, right? Should we update message and also throw an error if no argument is provided?
| await act(() => { | ||
| ReactNoop.render(null); | ||
| }); | ||
| assertLog(['destroy(2, Jack)']); |
| }); | ||
|
|
||
| // @gate enableUseResourceEffectHook | ||
| it('calls update on every render if no deps are specified', async () => { |
There was a problem hiding this comment.
Oh didn't realize we were supporting this case in my comment above. Should we require null to be more explicit, or would you rather follow the current useEffect pattern for now? If we are more explicit while introducing this, it will be easier to compile later.
| }); | ||
|
|
||
| // @gate enableUseResourceEffectHook | ||
| it('unmounts on deletion after skipped effect', async () => { |
There was a problem hiding this comment.
Check if this is relevant / clarify description
| @@ -92,6 +92,7 @@ export const retryLaneExpirationMs = 5000; | |||
| export const syncLaneExpirationMs = 250; | |||
| export const transitionLaneExpirationMs = 5000; | |||
| export const useModernStrictMode = true; | |||
| export const enableUseResourceEffectHook = false; | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduces a new experimental hook
useResourceEffect, which is something that we're doing some very early initial tests on.This may likely not pan out and will be removed or modified if so. Please do not rely on it as it will break.