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

Priority Queue: Fix for environments that don't have `window` defined #20486

Merged

Conversation

@p-jackson
Copy link
Contributor

p-jackson commented Feb 26, 2020

Description

Importing @wordpress/priority-queue, directly or indirectly, into an environment without window causes an error.

For example including the following in a test file so you can test a reducer:

const { combineReducers } = require( '@wordpress/data' );

and then running jest --env=node will fail with a "window is not defined" error.

The erroring code was added in #19282, and it looks like it was supposed to guard against this but there was just a typo.

How has this been tested?

I've tested this change against the test suite where I'm getting the errors and it resolves the issue.

Types of changes

Bug fix. I can't think of any situations where this would break existing code. The branch that includes the mock requestIdleCallback implementation is basically in a if ( false ) { ... } block at the moment, so I can't think of a way that any code would start using the wrong implementation.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@p-jackson p-jackson requested review from aduth and youknowriad as code owners Feb 26, 2020
@p-jackson p-jackson changed the title Priority Queue: Fix in environments that don't have `window` Priority Queue: Fix for environments that don't have `window` Feb 26, 2020
@p-jackson p-jackson changed the title Priority Queue: Fix for environments that don't have `window` Priority Queue: Fix for environments that don't have `window` defined Feb 26, 2020
@@ -2,7 +2,7 @@
* @return {typeof window.requestIdleCallback|typeof window.requestAnimationFrame|((callback:(timestamp:number)=>void)=>void)}
*/
export function createRequestIdleCallback() {
if ( typeof 'window' === undefined ) {
if ( typeof window === 'undefined' ) {

This comment has been minimized.

Copy link
@ramonjd

ramonjd Feb 27, 2020

Contributor

Nice spotting. Would there by any chance of getting this in for 7.6.0 @aduth ?

This comment has been minimized.

Copy link
@gziolo

gziolo Feb 27, 2020

Member

I can publish this fix to npm once merged.

This comment has been minimized.

Copy link
@sirreal

sirreal Feb 27, 2020

Member

Interesting that no-constant-condition lint rule didn't catch this, I suppose that's because undefined can be a bound name?

Copy link
Member

sirreal left a comment

Thanks! This looks like a straightforward bug fix.

It looks like this bug would prevent the fallback function from ever being used, so I wanted to verify this works as expected. I changed a few things to rely on the typechecker:

/**
 * @return {Window['requestIdleCallback'|'requestAnimationFrame']}
 */
export function createRequestIdleCallback() {
	if ( typeof window === 'undefined' ) {
		/**
		 * @param {Parameters<Window['requestAnimationFrame']>[0]} callback
		 */
		const requestIdleCallback = ( callback ) => {
			setTimeout( () => callback( Date.now() ), 0 );
			return -1;
		};
		return requestIdleCallback;
	}

	return window.requestIdleCallback || window.requestAnimationFrame;
}

export default createRequestIdleCallback();

Notes:

  • Narrow the type to satisfy Window['requestIdleCallback'|'requestAnimationFrame']
  • Type the returned function parameter
  • Date.now() is a number, but the requestIdleCallback callback parameter expects an IdleDeadline, which is more complex. number does satisfy the callback of requestAnimationFrame.
  • Returns a number handle. -1 seems safe as it shouldn't collide, however if window is undefined, and cancelIdleCallback doesn't exist, they're unlikely to ever be cancelled 🙂
@youknowriad youknowriad merged commit 7674245 into WordPress:master Feb 27, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 27, 2020
gziolo added a commit that referenced this pull request Feb 27, 2020
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 27, 2020

I cherry-picked this commit to wp/trunk to ensure it’s included in the next npm release. I believe @jorgefilipecosta will do the npm publish on Monday as part of the WordPress release cycle.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 27, 2020

Thanks all for managing this, and sorry for having introduced this careless error 😬

@p-jackson p-jackson deleted the p-jackson:fix/priority-queue-window-access branch Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.