Rewrite `afterLayoutFlush` as a generic TS function. #32926

sgomes · 2019-05-09T12:23:58Z

afterLayoutFlush now takes a function and returns a cancelable function of the same type. This is a stricter contract that helps avoid typing errors in consumers of this library.

This library seemed like a good opportunity to learn and test out more advanced TS concepts like generics, intersection types and union types.

Changes proposed in this Pull Request

Rewrite afterLayoutFlush as a generic TypeScript function.

Testing instructions

There are very few actual code changes, with most of this PR being about types. The unit tests should be enough to ensure that the library continues working normally.

matticbot · 2019-05-09T12:24:00Z

Test live: https://calypso.live/?branch=update/typed-after-layout-flush

matticbot · 2019-05-09T12:34:58Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~720 bytes added 📈)

name                   parsed_size           gzip_size
wp-super-cache               +24 B  (+0.0%)       +9 B  (+0.0%)
woocommerce                  +24 B  (+0.0%)      +11 B  (+0.0%)
themes                       +24 B  (+0.0%)      +17 B  (+0.0%)
theme                        +24 B  (+0.0%)      +14 B  (+0.0%)
stats                        +24 B  (+0.0%)      +12 B  (+0.0%)
settings-writing             +24 B  (+0.0%)       +9 B  (+0.0%)
settings-security            +24 B  (+0.0%)       +9 B  (+0.0%)
settings-performance         +24 B  (+0.0%)       +9 B  (+0.0%)
settings-discussion          +24 B  (+0.0%)       +9 B  (+0.0%)
settings                     +24 B  (+0.0%)       +9 B  (+0.0%)
security                     +24 B  (+0.0%)      +14 B  (+0.0%)
reader                       +24 B  (+0.0%)      +13 B  (+0.0%)
purchases                    +24 B  (+0.0%)      +11 B  (+0.0%)
posts-pages                  +24 B  (+0.0%)      +14 B  (+0.0%)
posts-custom                 +24 B  (+0.0%)      +12 B  (+0.0%)
post-editor                  +24 B  (+0.0%)      +12 B  (+0.0%)
plugins                      +24 B  (+0.0%)      +12 B  (+0.0%)
plans                        +24 B  (+0.0%)      +12 B  (+0.0%)
people                       +24 B  (+0.0%)      +18 B  (+0.0%)
notification-settings        +24 B  (+0.0%)      +14 B  (+0.0%)
media                        +24 B  (+0.0%)      +15 B  (+0.0%)
marketing                    +24 B  (+0.0%)       +9 B  (+0.0%)
gutenberg-editor             +24 B  (+0.0%)      +14 B  (+0.0%)
google-my-business           +24 B  (+0.0%)       +8 B  (+0.0%)
email                        +24 B  (+0.0%)      +12 B  (+0.0%)
earn                         +24 B  (+0.0%)       +9 B  (+0.0%)
domains                      +24 B  (+0.0%)      +14 B  (+0.0%)
comments                     +24 B  (+0.0%)      +13 B  (+0.0%)
checkout                     +24 B  (+0.0%)      +12 B  (+0.0%)
account                      +24 B  (+0.0%)      +11 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~168 bytes added 📈)

name                                         parsed_size           gzip_size
async-load-signup-steps-theme-selection            +24 B  (+0.1%)      +10 B  (+0.1%)
async-load-signup-steps-domains                    +24 B  (+0.0%)      +17 B  (+0.0%)
async-load-quick-language-switcher                 +24 B  (+0.1%)       +9 B  (+0.1%)
async-load-design-playground                       +24 B  (+0.0%)      +12 B  (+0.0%)
async-load-design-blocks                           +24 B  (+0.0%)      +16 B  (+0.0%)
async-load-design                                  +24 B  (+0.0%)      +11 B  (+0.0%)
async-load-components-web-preview-component        +24 B  (+0.0%)      +10 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

sgomes · 2019-05-09T14:10:51Z

This PR exposes an interesting problem: it appears that wp-desktop isn't set up to handle TypeScript correctly (see the failing wp-desktop tests) 😞

dmsnell · 2019-05-09T15:14:45Z

client/lib/after-layout-flush/index.ts

@@ -12,18 +14,23 @@
 * 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
+ * @returns {Function} The new delayed function


I've been removing the types from the JSDoc comments when adding them in the code. To me it makes sense not to keep two copies of that information next to each other when one will be tied to the code and the other manually updated. Already we have a divergence here because the JSDoc doesn't mention anything about being cancelable.

I actually did that at first, but it produces linting errors with our current config:

<path>/wp-calypso/client/lib/after-layout-flush/index.ts 5:1 error Missing JSDoc return type valid-jsdoc 16:4 error Missing JSDoc parameter type for 'func' valid-jsdoc

Should we remove JSDoc linting from TS files if we expect types to be omitted in the doc (which makes a lot of sense)?

that's odd. I thought I removed those lint lines. In #32872 I added requireParamType: false to our eslint config but it hasn't merged yet…

https://github.com/Automattic/wp-calypso/blob/master/packages/eslint-config-wpcalypso/index.js#L113

I suppose I could remove the JSDoc altogether and convert this to a standard comment, but that seems less than ideal, as I suspect IDEs won't pick it up.

Actually, I'll add a comment for now, to disable the valid-jsdoc rule throughout the file.

feel free to add that property to the eslint rule - I'm pretty sure we'll want it generally going forward, or create a new PR to add it, or wait and I'll try to make a PR to add it

client/lib/after-layout-flush/index.ts

dmsnell · 2019-05-09T16:40:46Z

client/lib/after-layout-flush/index.ts

-export default function afterLayoutFlush( func ) {
-	let rafHandle = undefined;
-	let timeoutHandle = undefined;
+export default function afterLayoutFlush< T extends ( ...args: any[] ) => any >( func: T ) {


this may be the equivalent of <T extends Function>

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.

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?

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.

dmsnell · 2019-05-09T17:58:09Z

client/lib/after-layout-flush/index.ts

 	};

 	// 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 );


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

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.

dmsnell · 2019-05-09T18:02:10Z

Does it seem like this could be easier if we collapsed the two halves of this function into one, whereby we only track one "scheduler handle" instead of two, and whereby we only have one wrapping function instead of two?

I'm sure I'm missing something, but why does the function work different in setTimeout environments than it does in requestAnimationFrame() environments? I see rafFunc() compared against timeoutFunc.apply( callThis, callArgs )

dmsnell · 2019-05-09T18:26:42Z

Doodling…

const [ schedule, clearSchedule ] = typeof requestAnimationFrame === 'function'
	? [ requestAnimationFrame, clearAnimationFrame ]
	: [ setTimeout, clearTimeout ];

export default afterLayoutFlush = f => {
	let handle: TimerHandle | undefined;

	const wrapped = ( ...args ) => {
		if ( handle ) {
			return;
		}

		handle = schedule( () => ( handle = undefined, f( ...args ) ) );
	}

	wrapped.cancel = () => ( clearSchedule( handle ), handle = undefined );

	return wrapped;
}

We impose the constraint that f must handle its own this, which from what I saw in the two uses of afterLayoutFlush() that I found is already the case, and forget about the accounting here.

I didn't verify, but when I see, for example, pendingLayoutFlush = afterLayoutFlush( this.checkScrollPosition ); and checkScrollPosition is defined as an arrow function it will be auto-bound to the proper this by way of our Babel transform.

any thoughts?

sgomes · 2019-05-14T21:10:43Z

Does it seem like this could be easier if we collapsed the two halves of this function into one, whereby we only track one "scheduler handle" instead of two, and whereby we only have one wrapping function instead of two?

Again, the correct flow is to apply both.

I'm sure I'm missing something, but why does the function work different in setTimeout environments than it does in requestAnimationFrame() environments? I see rafFunc() compared against timeoutFunc.apply( callThis, callArgs )

The raf code doesn't need to do as much, because the timeout code will still run (and always run, even if the raf code doesn't). These are stages, not alternative paths. Only on the final stage do we need to ensure that the saved attributes and this are applied, hence the apply call.

dmsnell · 2019-05-15T15:32:31Z

client/lib/after-layout-flush/index.ts

+}
+
+// NodeJS and the browser have different return types for `setTimeout`.
+type TimeoutHandle = ReturnType< typeof setTimeout >;


just a note: I've been adding these to client/types.ts

in #32940 I added this one specifically but it's blocked

would you be open to moving it there in here too? I can handle the rebase conflict later when #32940 is ready

No problem, I'll pull in your changes there 👍

dmsnell

@sgomes thanks for doing this. putting this out there should be good and even if we refine the types I think that getting these changes out first will help

I've requested that you move TimeoutHandle into client/types.ts but you can ignore that. if you do ignore it would you mind adding a @TODO that I can find when I consolidate those global types later?

sgomes · 2019-05-15T17:17:23Z

@dmsnell Thank you for the review! Do you think this is ready for a merge despite the wp-desktop breakage, or do we need to get TS support into wp-desktop first?

jsnajdr · 2019-05-16T12:01:12Z

Do you think this is ready for a merge despite the wp-desktop breakage

We usually don't break wp-desktop intentionally. If we did, how are we supposed to build and release the next version?

sgomes · 2019-05-16T15:17:36Z

We usually don't break wp-desktop intentionally. If we did, how are we supposed to build and release the next version?

I figured as much, but wasn't sure, since it's common for the tests to break. I'll keep this PR open until TS support gets added to wp-desktop, then.

dmsnell · 2019-05-16T17:08:20Z

It took me a day here to think about it and yeah I can't recommend breaking desktop. Initially I overlooked that part. Is it broken already with the iframe bridge?

Maybe we can all try and figure it out - get it building fine this week.

dmsnell · 2019-05-17T22:40:22Z

@sgomes I've created Automattic/wp-desktop#598 to try and get on top of the desktop app. not sure if what I'm seeing over there is even an existing problem with my setup but I think some of my changes at least are essential in any case

sirreal · 2019-05-17T23:02:01Z

I started poking at updating the desktop build in Automattic/wp-desktop#599. It's going to take some effort, help appreciated.

sirreal · 2019-05-28T11:16:34Z

I've got a fix for TypeScript in wp-desktop: Automattic/wp-desktop#604

The tests are passing here because I rebased the desktop test branch on that PR ☝️. This still shouldn't be merged until that lands.

sgomes · 2019-05-28T11:37:45Z

Woohoo, thanks, @sirreal ! I'll wait until Automattic/wp-desktop#604 is merged :)

sirreal · 2019-05-28T15:06:14Z

We'll probably need a rebase for Desktop to be really happy to get #33230. Related: should Desktop (electron) use the evergreen build?

sgomes · 2019-05-28T15:09:39Z

Related: should Desktop (electron) use the evergreen build?

No. This is how I set it up initially, but I ended up changing it later on. Electron does not necessarily offer all the features that the evergreen build may expect to be present, particularly since I don't think we update the Electron dependency in wp-desktop very often. It's safer for it to stay with the fallback build (and not a huge perf concern there).

sirreal · 2019-05-28T15:24:20Z

Automattic/wp-desktop#604 has landed. Let the TypeScript flow 😎

sgomes · 2019-05-28T15:25:13Z

Woohoo, thanks @sirreal ! I'm rebasing this on master to make sure nothing's broken.

It now takes a function and returns a cancelable function of the same type. This is a stricter contract that helps avoid typing errors in consumers of this library.

sgomes · 2019-05-28T16:09:03Z

Seems to be working fine 🎉

Want to give this a final look-over, @sirreal ?

sirreal

sgomes added [Type] Enhancement [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 9, 2019

sgomes requested review from jsnajdr and a team May 9, 2019 12:23

dmsnell reviewed May 9, 2019

View reviewed changes

client/lib/after-layout-flush/index.ts Outdated Show resolved Hide resolved

dmsnell reviewed May 9, 2019

View reviewed changes

dmsnell reviewed May 15, 2019

View reviewed changes

dmsnell approved these changes May 15, 2019

View reviewed changes

sgomes force-pushed the update/typed-after-layout-flush branch from 26a77b8 to 542fbd3 Compare May 15, 2019 16:54

sgomes force-pushed the update/typed-after-layout-flush branch from 542fbd3 to 7d4f190 Compare May 27, 2019 13:10

sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 28, 2019

sirreal mentioned this pull request May 28, 2019

Build: Handle TypeScript Automattic/wp-desktop#604

Merged

sirreal added the [Status] Blocked / Hold label May 28, 2019

sirreal assigned sgomes May 28, 2019

sgomes added 6 commits May 28, 2019 16:25

Rewrite afterLayoutFlush as a generic TS function.

9e42df4

It now takes a function and returns a cancelable function of the same type. This is a stricter contract that helps avoid typing errors in consumers of this library.

Remove redundant (and incorrect) JSDoc types

fb5e6a0

Remove redundant return type from function.

1ef3c38

Set timeout handle type more explicitly

b660050

Use global type for timer handle.

166a47d

Remove ESLint disabling.

884ae27

sgomes force-pushed the update/typed-after-layout-flush branch from 7d4f190 to 884ae27 Compare May 28, 2019 15:26

sirreal approved these changes May 28, 2019

View reviewed changes

sgomes merged commit 96d57d6 into master May 29, 2019

sgomes deleted the update/typed-after-layout-flush branch May 29, 2019 10:40

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 29, 2019

Rewrite afterLayoutFlush as a generic TS function. #32926

Rewrite afterLayoutFlush as a generic TS function. #32926

Conversation

sgomes commented May 9, 2019

Changes proposed in this Pull Request

Testing instructions

matticbot commented May 9, 2019

matticbot commented May 9, 2019 • edited

sgomes commented May 9, 2019

Choose a reason for hiding this comment

sgomes May 9, 2019 • edited

Choose a reason for hiding this comment

dmsnell May 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented May 9, 2019

dmsnell commented May 9, 2019

sgomes commented May 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell left a comment

Choose a reason for hiding this comment

sgomes commented May 15, 2019

jsnajdr commented May 16, 2019

sgomes commented May 16, 2019

dmsnell commented May 16, 2019

dmsnell commented May 17, 2019

sirreal commented May 17, 2019

sirreal commented May 28, 2019

sgomes commented May 28, 2019

sirreal commented May 28, 2019

sgomes commented May 28, 2019

sirreal commented May 28, 2019

sgomes commented May 28, 2019

sgomes commented May 28, 2019

sirreal left a comment

Choose a reason for hiding this comment

Rewrite `afterLayoutFlush` as a generic TS function. #32926

Rewrite `afterLayoutFlush` as a generic TS function. #32926

matticbot commented May 9, 2019 •

edited

sgomes May 9, 2019 •

edited

dmsnell May 9, 2019 •

edited