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: call dynamic hooks in a way that can be compiled #6814

Merged
merged 2 commits into from
Jun 1, 2024

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented May 30, 2024

Description

Refactors FieldAction and InspectorMenuItem components so they're no longer calling hooks that come from props. Instead, the components themselves are dynamically defined, allowing React to call said underlying hooks in a dynamic way.
It upholds the guarantee that no matter how many times, say, FieldAction re-renders, it'll never suddenly see props.action.useAction() change identity, because if it changes a new component is created instead.

What to review

It should make sense. There's another linter warning in DocumentInspectorMenuItemsResolver.tsx that will be handled in a follow up.

Testing

Existing tests should be enough.

Notes for release

N/A

@stipsan stipsan requested a review from a team as a code owner May 30, 2024 15:15
@stipsan stipsan requested review from bjoerge and removed request for a team May 30, 2024 15:15
Copy link

vercel bot commented May 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 2:21pm
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 2:21pm
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 2:21pm
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 2:21pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 2:21pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview May 31, 2024 2:21pm

Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented May 30, 2024

Component Testing Report Updated May 31, 2024 2:27 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 35s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 26s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 31s 11 7 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 36s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 16s 19 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 2s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 6s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 20s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 29s 12 0 0

Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

Great! Looks like there's some unit tests/snapshots that needs updating though

@stipsan stipsan force-pushed the fix-dynamic-hooks-so-they-can-compile branch from b20a4af to 99a792f Compare May 31, 2024 09:50
Copy link

socket-security bot commented May 31, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/@react-three/drei@9.106.0, npm/@repo/package.bundle@3.44.0, npm/@repo/package.config@3.44.0, npm/@repo/shared-modules.bundle@3.44.0, npm/@repo/test-exports@3.44.0, npm/@repo/tsconfig@3.44.0, npm/@sanity/block-tools@3.0.0-v3-pte.88+76383ff4e, npm/@sanity/block-tools@3.44.0, npm/@sanity/cli@3.44.0, npm/@sanity/codegen@3.44.0, npm/@sanity/diff@3.44.0, npm/@sanity/eventsource@3.0.3, npm/@sanity/migrate@3.44.0, npm/@sanity/mutator@3.44.0, npm/@sanity/portable-text-editor@3.44.0, npm/@sanity/preview-url-secret@1.6.17, npm/@sanity/schema@3.0.0-v3-pte.88+76383ff4e, npm/@sanity/schema@3.44.0, npm/@sanity/types@3.0.0-v3-pte.88+76383ff4e, npm/@sanity/util@3.0.0-v3-pte.88+76383ff4e, npm/@sanity/vision@3.44.0, npm/async@2.6.4, npm/balanced-match@0.4.2, npm/big.js@3.2.0, npm/bn.js@4.12.0, npm/browserslist@2.11.3, npm/caniuse-api@3.0.0, npm/create-sanity@3.44.0, npm/emojis-list@2.1.0, npm/fast-deep-equal@3.1.3, npm/groq@3.44.0, npm/inherits@2.0.3, npm/is-descriptor@0.1.7, npm/is-extendable@0.1.1, npm/is-extendable@1.0.1, npm/is-plain-object@5.0.0, npm/js-tokens@4.0.0, npm/json-schema-traverse@0.4.1, npm/json5@0.5.1, npm/kind-of@3.2.2, npm/loader-utils@2.0.4, npm/mime@1.6.0, npm/ms@2.0.0, npm/ms@2.1.3, npm/nano-pubsub@1.0.2, npm/nanoid@3.3.7, npm/object-assign@4.1.1, npm/p-locate@4.1.0, npm/path-exists@4.0.0, npm/perf-studio@3.44.0, npm/picocolors@0.2.1, npm/picocolors@1.0.1, npm/pify@2.3.0, npm/postcss-selector-parser@2.2.3, npm/postcss-selector-parser@3.1.2, npm/postcss-value-parser@4.2.0, npm/postcss@6.0.23, npm/postcss@8.4.38, npm/punycode@1.4.1, npm/react-is@16.13.1, npm/react@16.14.0, npm/readable-stream@2.3.8, npm/resolve-from@3.0.0, npm/safe-buffer@5.1.2, npm/safe-buffer@5.2.1, npm/sanity-perf-tests@3.44.0, npm/sanity@3.44.0, npm/semver@5.7.2, npm/shallowequal@1.1.0, npm/source-map-js@1.2.0, npm/source-map@0.5.7, npm/strip-ansi@3.0.1, npm/styled-components@5.3.11, npm/stylis@4.3.2, npm/supports-color@2.0.0, npm/tslib@2.6.2, npm/yallist@2.1.2

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

@stipsan
Copy link
Member Author

stipsan commented May 31, 2024

@SocketSecurity ignore-all

@stipsan
Copy link
Member Author

stipsan commented May 31, 2024

@SocketSecurity ignore-all

@stipsan stipsan added this pull request to the merge queue Jun 1, 2024
Merged via the queue into next with commit 6e65eed Jun 1, 2024
44 checks passed
@stipsan stipsan deleted the fix-dynamic-hooks-so-they-can-compile branch June 1, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants