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

ESLint plugin: Remove temporary listed types for TypeScript validation (p. 1) #18808

Merged
merged 4 commits into from Dec 3, 2019

Conversation

@gziolo
Copy link
Member

gziolo commented Nov 28, 2019

Description

Follow-up for #18025 where I introduced a temporary list of whitelisted internal JSDoc type definitions to make ESLint happy.

This work is inspired by the blog post from @aduth:
https://andrewduthie.com/2019/11/27/typescript-tooling-for-your-javascript-projects/

In particular this part:

I suggest defining a typedef to serve as an alias to the imported type. In this way, it acts like any other import statement at the top of your file.

/** @typedef {import('./tasks').Task} Task */

With this alias in place, you can then proceed to reference the type as Task once more.

This PR doesn't refactor all type definitions. It's quite a manual task so I decided to divide it into at least two parts.

How has this been tested?

npm run lint-js

@gziolo gziolo requested a review from aduth Nov 28, 2019
@gziolo gziolo self-assigned this Nov 28, 2019
@gziolo gziolo changed the title ESLint plugin: Remove temporary listed types for TypeScript validation ESLint plugin: Remove temporary listed types for TypeScript validation (p. 1) Nov 28, 2019
packages/notices/src/store/selectors.js Outdated Show resolved Hide resolved
@@ -15,6 +15,8 @@ import { compose } from '@wordpress/compose';
*/
import withRegistryProvider from './with-registry-provider';

/** @typedef {import('@wordpress/data').WPDataRegistry} WPDataRegistry */

This comment has been minimized.

Copy link
@aduth

aduth Nov 29, 2019

Member

It's hard to tell.. are these actually resolving to anything? When I evaluate them in the editor, it just shows as the fallback "any" type.

image

It's something where I think just like our modules have a public API for functions, etc, we'll want to see how to expose the types as part of the public interface. It seems like this can work by default when the typedef is defined in the entry point file of a module, but if it's not, we'll need to figure out a way to "re-export" it (I think it's enough to do the typedef aliasing in the entry point file).

I guess in any case, this helps avoid having the whitelist while still making the linter happy, then we can solve the problem of the public types definitions separately.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 30, 2019

Author Member

I need to start using the proper IDE first to feel the experience 😂

How about we put all type definitions in the root index.js file of each package and import in all places where they are used? It should make it easier to manage.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 2, 2019

Author Member

I see what you mean. The issue I see is that sometimes those imports are resolved with DefinitelyTyped, so I guess it should be tackled separately. Well, I opened Visual Studio Code for the first time since I don't remember 😅

I tried two things:

  • re-import the existing definition from the index.js file but it didn't work
  • moving definitions to index.js and referencing them for @wordpress/data from there worked for all packages, but it didn't work for @wordpress/element when I used the same technique
@@ -3,12 +3,14 @@
*/
import { first } from 'lodash';

/** @typedef {import('./puppeteer').ElementHandle} ElementHandle */

This comment has been minimized.

Copy link
@aduth

aduth Nov 29, 2019

Member

Is this meant to be a relative path? Or should it be import('puppeteer') ?

I think once we have these files included in tsconfig.json, we should start to get errors for these issues.

image

Suggested change
/** @typedef {import('./puppeteer').ElementHandle} ElementHandle */
/** @typedef {import('puppeteer').ElementHandle} ElementHandle */

Technically the above won't work correctly either, at least not without also installing @types/puppeteer, since Puppeteer doesn't publish its own types.

But with @types/puppeteer installed, it works like a charm:

image

Fun fact: By temporarily adding this file to the includes set, it highlighted a legitimate error in our @return statement a few lines below this one (seen in the last screenshot). Since the function is async, it doesn't return an ElementHandle, it returns a Promise<ElementHandle>.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 30, 2019

Author Member

It was a typo, the part which references Puppeteer. Can you push changes directly to this branch? I’m still learning all that stuff. My very first task for next week is to migrate to Visual Studio Code 😜

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 2, 2019

Author Member

I fixed imports for both puppeteer and moment.

Can we handle the individual issues as we enabled TS validation?

@gziolo gziolo force-pushed the update/eslint-jsdoc-typescript branch from 2b1d80a to d8d3c5a Dec 2, 2019
@aduth aduth mentioned this pull request Dec 2, 2019
5 of 65 tasks complete
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Dec 2, 2019

Can we split efforts into 3 PRs:

  • this PR
  • a similar follow-up PR for remaining local type definitions
  • a final PRs which ensures that packages can properly import types from other packages

What do you think?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 2, 2019

Since it starts to touch on what's tracked in #18838 with regards to opting packages into type-checking, I would add to be considerate of potential issues when adding these references to types locally in files.

Temporarily adding // @ts-check to the top of data/src/index.js, for example, highlights many interesting legitimate issues, and to the specific point of the Moment typedef you've added, has some impact on whether this is the correct type to be using. For example, we use and rely on moment.tz from the moment-timezone package, which isn't technically part of the Moment interface.

But in general, yes, I think we should be as open as possible to trying to make this process iterative, than to try to add barriers to what will otherwise already be a significant undertaking.

@aduth
aduth approved these changes Dec 2, 2019
packages/e2e-test-utils/README.md Show resolved Hide resolved
@gziolo gziolo force-pushed the update/eslint-jsdoc-typescript branch from c247f1d to 4cd89a9 Dec 3, 2019
@gziolo gziolo merged commit c8c6903 into master Dec 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo deleted the update/eslint-jsdoc-typescript branch Dec 3, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Dec 3, 2019

Temporarily adding // @ts-check to the top of data/src/index.js, for example, highlights many interesting legitimate issues, and to the specific point of the Moment typedef you've added, has some impact on whether this is the correct type to be using.

So this is how you test it. It would be worth adding a note in #18838 with how you can enable such checks when working on enabling validation. It might be a good way to do the transition gradually in the case where packages have multiple files that error.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.