Fix undefined window in Resizable-box tooltip #38400
Conversation
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @kuuak! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
kuuak
force-pushed
the
fix/resizablebox-tooltip-utils-window-undefined
branch
from
778ecea to
8768922
Compare
| @@ -9,7 +9,8 @@ import useResizeAware from 'react-resize-aware'; | |||
| */ | |||
| import { useEffect, useRef, useState } from '@wordpress/element'; | |||
|
|
|||
| const { clearTimeout, setTimeout } = window; | |||
| const { clearTimeout = () => undefined, setTimeout = () => undefined } = | |||
There was a problem hiding this comment.
Once this PR is rebased the noop function will no longer be imported from lodash but this file will define its own. If we change the noop definition from () => {} to () => undefined then you'll be able to set the fallback values for clearTimeout and setTimeout to the shared noop function.
There was a problem hiding this comment.
@jsnajdr suggested we could use globalThis here instead of window.
e.g. something like:
| const { clearTimeout = () => undefined, setTimeout = () => undefined } = | |
| const { clearTimeout, setTimeout } = globalThis; |
There was a problem hiding this comment.
Apparently we can't use the globalThis trick in cases where we need to use the return of the setTimeout call, because in Node it returns NodeJS.Timeout instead of number.
In this code it won't ever actually run in a SSR context because the call is inside a useEffect, but the TypeScript checker doesn't know that, so it kindly throws an error 😄
There was a problem hiding this comment.
Would something like this work?
const moveTimeoutRef = useRef< ReturnType< typeof setTimeout > >()
Potentially might need some nulls.
There was a problem hiding this comment.
Can't we just refactor the code so that we can write window.setTimeout and window.clearTimeout inline inside the setTimeout ?
There was a problem hiding this comment.
Thank you for contributing, and sorry this took so much time to get onto our radar! I proposed #42248 to prevent further regressions, and to fix up the remaining issues.
@kuuak If you are still available to move forward with this PR, it'd be great if you could rebase and add a changelog. Just let us know if you don't have time, and we can batch fix it in #42248 👍
| @@ -9,7 +9,8 @@ import useResizeAware from 'react-resize-aware'; | |||
| */ | |||
| import { useEffect, useRef, useState } from '@wordpress/element'; | |||
|
|
|||
| const { clearTimeout, setTimeout } = window; | |||
| const { clearTimeout = () => undefined, setTimeout = () => undefined } = | |||
There was a problem hiding this comment.
Apparently we can't use the globalThis trick in cases where we need to use the return of the setTimeout call, because in Node it returns NodeJS.Timeout instead of number.
In this code it won't ever actually run in a SSR context because the call is inside a useEffect, but the TypeScript checker doesn't know that, so it kindly throws an error 😄
|
We fixed this issue along with the other existing SSR issues, so we could merge eslint rules to prevent further regressions (#42248). Apologies again for letting this sit so long! |
Description
Following the refactor and transformation of the resisable-box to typescript (PR #35062) the window object were not anymore tested to be
undefined. (exactly here)This PR revert this single line of code to verify wether
windowisundefinedand sets default toclearTimeoutandsetTimeoutto avoid any crash of the app using @wordpress/components packagesWe use @wordpress/components in the frontend of our app built in nextjs. Therefore when building the app or generating a static version on the server, the window object is not accessible. Hence the bug encountered when updating the package to its latest version which removed the
undefinedtest of the window object.Types of changes
Checklist:
[ ] My code follows the accessibility standards.not relevant as no changes made on displayed component[ ] I've tested my changes with keyboard and screen readers.not relevant[ ] My code has proper inline documentation.not relevant[ ] I've included developer documentation if appropriate.not relevant[ ] I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search allnot relevant*.native.jsfiles for terms that need renaming or removal).[ ] I've updated related schemas if appropriate.not relevant