Components: refactor Sandbox to pass exhaustive-deps#44378
Merged
Conversation
chad1008
added
[Type] Code Quality
44 tasks
|
Size Change: -3 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 23, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this, @chad1008
Unfortunately I think that most of the suggested changes would make the Sandbox component behave in a different way to how it was intended to. In order to keep using useEffects like today and respect the exhaustive deps rule, we'd probably need to refactor the code heavily (and probably rely of refs holding the previous value of some props).
Probably the best thing to do here is to add eslint-ignore comments, as I'm afraid that a proper fix could be tricky to write, tricky to test and could take some time to apply.
Also cc'ing @ellatrix who mostly worked on this component, in case she has any other suggestions on how to rewrite this component respecting the exhaustive-deps rule.
| ref.current.addEventListener( 'load', tryNoForceSandbox, false ); | ||
| defaultView.addEventListener( 'message', checkMessageForResize ); | ||
|
|
||
| const iframeRef = ref.current; |
Contributor
There was a problem hiding this comment.
We should probably declare this variable a few lines up, and use it also instead for the ref.current.addEventListener call?
Contributor
There was a problem hiding this comment.
diff --git a/packages/components/src/sandbox/index.js b/packages/components/src/sandbox/index.js
index 6bf9669613..67929c6e4e 100644
--- a/packages/components/src/sandbox/index.js
+++ b/packages/components/src/sandbox/index.js
@@ -205,22 +205,19 @@ export default function Sandbox( {
setHeight( data.height );
}
- const { ownerDocument } = ref.current;
+ const iframe = ref.current;
+ const { ownerDocument } = iframe;
const { defaultView } = ownerDocument;
// This used to be registered using <iframe onLoad={} />, but it made the iframe blank
// after reordering the containing block. See these two issues for more details:
// https://github.com/WordPress/gutenberg/issues/6146
// https://github.com/facebook/react/issues/18752
- ref.current.addEventListener( 'load', tryNoForceSandbox, false );
+ iframe.addEventListener( 'load', tryNoForceSandbox, false );
defaultView.addEventListener( 'message', checkMessageForResize );
return () => {
- ref.current?.removeEventListener(
- 'load',
- tryNoForceSandbox,
- false
- );
+ iframe?.removeEventListener( 'load', tryNoForceSandbox, false );
defaultView.addEventListener( 'message', checkMessageForResize );
};
}, [] );
Contributor
Author
There was a problem hiding this comment.
Good point! I'll hold off on this for now, since we're going with ignore-rule comments instead for the time being, but thank you for sharing it with me!
Contributor
Author
There was a problem hiding this comment.
...actually, scratch that. I've included this change in PR, as it isn't subject to the same complications as the useEffect dependencies.
Comment on lines
+230
to
+238
| }, [ trySandbox ] ); | ||
|
|
||
| useEffect( () => { | ||
| trySandbox(); | ||
| }, [ title, styles, scripts ] ); | ||
| }, [ trySandbox ] ); | ||
|
|
||
| useEffect( () => { | ||
| trySandbox( true ); | ||
| }, [ html, type ] ); | ||
| }, [ trySandbox ] ); |
Contributor
There was a problem hiding this comment.
I'm afraid that the refactor around trySandbox doesn't make much sense — it looks like the author's original intent was to pass true to the trySandbox call only when html and type changed — with the current changes in this PR, trySandbox would be called with both undefined and true everytime its value changes.
| defaultView.addEventListener( 'message', checkMessageForResize ); | ||
| }; | ||
| }, [] ); | ||
| }, [ trySandbox ] ); |
Contributor
There was a problem hiding this comment.
This useEffect was supposed to run only when the component mounts, but with the changes from this PR it will run every time trySandbox changes — i.e any time any of its dependencies change.
Contributor
Author
|
Thanks for the feedback Marco - this one was definitely harder to parse/test, so you're probably right about simply ignoring for now, and refactoring later. I'll update the PR to reverse the proposed changes and apply some |
…ame Ref declaration
chad1008
force-pushed
the
refactor/Sandbox-exhaustive-deps
branch
from
1967c7a to
eec8709
Compare
ciampo
approved these changes
Sep 26, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
LGTM for now. We can always come back to this and attempt to rewrite this component, likely in a way that avoids a few useEffect calls
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
What?
Updates the
Sandboxcomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
ref.currentwas originally called in theuseEffectcleanup function, but it's possible/likely that value will change before the cleanup is run. Because the ref points to a node rendered by React, the linter recommends copying the ref value to a variable inside the effect, and then referencing that variable in the cleanup function.trySandboxwas a missed dependency for several effects, so it's now wrapped in auseCallback.The last two
useEffectspreviously had some of the component's props as dependencies, even though the don't actually appear in the effect. Now that those props will causetrySandboxto be redeclared, andtrySandboxis a dependency of those effects, I've removed those old deps to clean up the arrays.Last but not least, an ignore comment has been added to the one native file that was throwing a warning so this can be refactored by the native team.
Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/sandbox