Components: refactor Sandbox to pass exhaustive-deps
#44378
Merged
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
chad1008
added
[Type] Code Quality
44 tasks
|
Size Change: -3 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Sep 23, 2022
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.
| @@ -215,23 +221,21 @@ export default function Sandbox( { | |||
| ref.current.addEventListener( 'load', tryNoForceSandbox, false ); | |||
| defaultView.addEventListener( 'message', checkMessageForResize ); | |||
|
|
|||
| const iframeRef = ref.current; | |||
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?
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 );
};
}, [] );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!
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 ] ); |
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 ] ); |
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.
|
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
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.
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