search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.51k stars 4.2k forks source link

Testing: Add TypeScript types validation for modules #18838

Closed aduth closed 7 months ago

aduth commented 4 years ago

Previously: #17014

While Gutenberg packages are not authored in TypeScript, we can still benefit from its JavaScript type checking using the JSDoc we already write. This will bring us some benefit of type safety, even as we continue to write modules with JavaScript, not TypeScript.

Progress:

All of our modules are to be opted-in to type checking. The progress thus far:

Guidelines:

See the packages README for details on TypeScript usage. In summary, a package can opt-in by following these steps:

For single files, you can also include a // @ts-check line at the top of the file. You should use this line when temporarily testing the impact of type checking, or if you are working to convert a very large package one file at a time.

The build:package-types npm script will be run as part of precommit, which handles type checking. It can be very helpful to watch the types build and work through issues surfaced by the compiler as you add types. To watch the types build, run:

npm run build:package-types -- --watch

You may also need to improve existing JSDoc, as it was written based on assumed usage. Type checking will enforce its validity, and in many cases our JSDoc is invalid.

For syntax and usage, reference:

aduth commented 4 years ago

cc @dsifford

dsifford commented 4 years ago

It should be okay to use DefinitelyTyped (@types/*) to reference types of external packages

This is good guidance for now while this is being worked on, but once the repo gets all of the JSDoc finished up I would suggest referencing your internal JSDoc definitions to keep things consistent. This can be done using tsconfig.json's paths configuration. External types can still and should still be referenced where appropriate.

So for example...

The tsconfig.json would be modified with the following new values...

{
  "compilerOptions": {
    "baseUrl": "./packages",
    "paths": {
      "@wordpress/*": [
        "*"
      ]
    }
  }
}

And then you'd be able to import types the same as before...

// Assume this is in a file from some other package that isn't @wordpress/element
/**
 * @typedef {import('@wordpress/element').ComponentType} ComponentType
 */

(this is off the top of my head, I may be slightly incorrect in the exact implementation, but you should get the idea).

aduth commented 4 years ago

@dsifford Yes, definitely, I think for WordPress dependencies we should aim for what you propose.

There was some previous related discussion to this:

dsifford commented 4 years ago

Yep, that --declaration flag is a pretty big game-changer. Though, the types still won't be quite as good as those written in typescript due to limited support for generics and overloads.

sirreal commented 4 years ago

I've started exploring generated .d.ts declarations in #18942.

As part of that PR, I typed i18n and is-shallow-equal. Shall I extract the changes to those modules so they can be landed?

aduth commented 4 years ago

As part of that PR, I typed i18n and is-shallow-equal. Shall I extract the changes to those modules so they can be landed?

If you're willing to, I think that would make review of the distinct parts much easier, yes.

DanielRosenwasser commented 4 years ago

This seems like a potentially good opportunity for our team (TypeScript) to get some feedback! Let us know if you'd like to chat at some point, or just have any feedback on adding checkJs validation.

Also, CC @weswigham and @sandersn in case you're interested in a fairly large JSDoc'd codebase!

aduth commented 4 years ago

@DanielRosenwasser Thanks for offering your assistance and for being receptive to the feedback!

Speaking for myself in having implemented JSDoc-based types enforcement for a few of the packages in this project (and written about and used this approach in a few of my own personal projects), I've generally been pleasantly surprised with how well it "just works".

The struggles we've encountered thus far are usually not a fault of TypeScript itself, but moreso on interoperability with other tooling. For example, we use eslint-plugin-jsdoc to validate our JSDoc, but because this doesn't yet have complete support for TypeScript types syntaxes, it often leads to a number of false positives. Similarly, we've implemented our own automated documentation tool which uses the (now-defunct) Doctrine parser, which also fails to understand this syntax (see also #19571, #18045). In general, I think this speaks more to the insufficiencies of the base JSDoc standard, which in turn have motivated various syntax extensions (TypeScript, Closure Compiler), ultimately resulting in a mixed landscape of support for "JSDoc" as somewhat ambiguous in its interpreted standard.

There's been some challenge in learning and applying these syntax extensions, and finding a good balance of just how far we want to take it. I'm sure this is a common theme in any project which implements TypeScript (especially migrating from JavaScript), but I often find it more acute in using the TypeScript syntaxes in JSDoc, where it has a greater impact on line-length and readability (trying to fit the type definition within a single line @param type, for example). It might be a curious thing to concern about, but I'd wonder if it might influence how people choose to use types, and consequently the effectiveness in how we leverage this tooling. I suspect these are growing pains which will evolve over time, including with improved guidance around applying these conventions (e.g. #18920).

In case there's interest in the history surrounding this project's relationship with TypeScript, it's been a topic of discussion in several of WordPress' weekly JavaScript meetings:

sirreal commented 4 years ago

I've updated the description according to current setup now that #18942 has landed. I'm unsure whether this note still applies, but I've left it:

For single files, you can also include a // @ts-check line at the top of the file. You should use this line when temporarily testing the impact of type checking, or if you are working to convert a very large package one file at a time.

annezazu commented 7 months ago

Closing this out as the work has either been completed or is no longer relevant. Happy to reopen if we need to but, at this point, a new issue is likely best.