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.

Sign up for GitHub

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

Jump to bottom

Rewrite afterLayoutFlush as a generic TS function. #32926

Merged
merged 6 commits into from May 29, 2019
Merged

Rewrite afterLayoutFlush as a generic TS function. #32926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e42df4
Rewrite afterLayoutFlush as a generic TS function.
sgomes May 9, 2019
fb5e6a0
Remove redundant (and incorrect) JSDoc types
sgomes May 9, 2019
1ef3c38
Remove redundant return type from function.
sgomes May 9, 2019
b660050
Set timeout handle type more explicitly
sgomes May 9, 2019
166a47d
Use global type for timer handle.
sgomes May 15, 2019
884ae27
Remove ESLint disabling.
sgomes May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 26 additions & 17 deletions client/lib/after-layout-flush/index.js → client/lib/after-layout-flush/index.ts
View file
Open in desktop
@@ -1,4 +1,11 @@
/** @format */
/**
* Internal dependencies
*/
import { TimerHandle } from 'types';

interface Cancelable {
cancel: () => void;
}

/**
* Creates a delayed function that invokes `func` as soon as possible after the next layout
Expand All @@ -11,19 +18,20 @@
* Inspired by the Firefox performance best practices MDN article at:
* https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers
*
* @param {Function} func The function to be invoked after the layout flush
* @returns {Function} Returns the new delayed function
* @param func - The function to be invoked after the layout flush
* @returns The new delayed function
*/
export default function afterLayoutFlush( func ) {
let rafHandle = undefined;
let timeoutHandle = undefined;
export default function afterLayoutFlush< T extends ( ...args: any[] ) => any >( func: T ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this may be the equivalent of <T extends Function>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too, but switching to that produces the following error further down in the scheduleRAF and scheduleTimeout returns:

Conversion of type '(this: any, ...args: any[]) => void' to type 'T' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for the delay - it's something we'd have to coordinate below.

also, can you help me understand the purpose of lastThis and the like? what are those there fore and why are we using them? are we having trouble with this binding and maintaining context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afterLayoutFlush is meant to be as bullet-proof as possible with regards to this usage, similar to defer and friends in Lodash. Because of that, we store and restore the this we receive, so that whatever the context the function is used in, everything will still work.

let timeoutHandle: TimerHandle | undefined = undefined;
let rafHandle: number | undefined = undefined;

const hasRAF = typeof requestAnimationFrame === 'function';

let lastThis, lastArgs;
let lastThis: any;
let lastArgs: any[] | undefined;

const scheduleRAF = function( rafFunc ) {
return function( ...args ) {
const scheduleRAF = function( rafFunc: T ) {
return function( this: any, ...args: any[] ) {
lastThis = this;
lastArgs = args;

Expand All @@ -37,11 +45,11 @@ export default function afterLayoutFlush( func ) {
rafHandle = undefined;
rafFunc();
} );
};
} as T;
};

const scheduleTimeout = function( timeoutFunc ) {
return function( ...args ) {
const scheduleTimeout = function( timeoutFunc: T ) {
return function( this: any, ...args: any[] ) {
if ( ! hasRAF ) {
lastThis = this;
lastArgs = args;
Expand All @@ -55,7 +63,7 @@ export default function afterLayoutFlush( func ) {
}

timeoutHandle = setTimeout( () => {
const callArgs = lastArgs;
const callArgs = lastArgs !== undefined ? lastArgs : [];
const callThis = lastThis;

lastArgs = undefined;
Expand All @@ -65,17 +73,18 @@ export default function afterLayoutFlush( func ) {
timeoutHandle = undefined;
timeoutFunc.apply( callThis, callArgs );
}, 0 );
};
} as T;
};

// if RAF is not supported (in Node.js test environment), the wrapped function
// will only set a timeout.
let wrappedFunc = scheduleTimeout( func );
let wrappedFunc: T = scheduleTimeout( func );
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we always create the timeout function even when requestAnimationFrame() is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct flow is to go through both. The article linked to in the file explains why, exactly, but it basically boils down to browser rendering timing quirks.

When requestAnimationFrame isn't available, we do a best effort of running through just scheduleTimeout, which is better than nothing.

if ( hasRAF ) {
wrappedFunc = scheduleRAF( wrappedFunc );
}

wrappedFunc.cancel = () => {
const wrappedWithCancel = wrappedFunc as T & Cancelable;
wrappedWithCancel.cancel = () => {
lastThis = undefined;
lastArgs = undefined;

Expand All @@ -90,5 +99,5 @@ export default function afterLayoutFlush( func ) {
}
};

return wrappedFunc;
return wrappedWithCancel;
}
5 changes: 5 additions & 0 deletions client/types.ts
View file
Open in desktop
Expand Up @@ -18,3 +18,8 @@ export type PostType = 'page' | 'post' | string;

// Comment stuff
export type CommentId = number;

// Language stuff
export type Lazy< T > = () => T;
export type TimestampMS = ReturnType< typeof Date.now >;
export type TimerHandle = ReturnType< typeof setTimeout >;