search

WordPress / gutenberg

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

Refactor `@wordpress/components` package to TypeScript #35744

Closed andrewserong closed 9 months ago

andrewserong commented 2 years ago

Related to #30503, #28399 and #27484

Context ✨

We would like to refactor the entire @wordpress/components package to TypeScript.

Some components are already written in TypeScript, while the majority of the code is still written in JavaScript.

By refactoring the whole package to TypeScript, we would be able to take advantage of type safety, while also providing first-party types to the package's consumers (instead of the third-party ones).

A fully-typed set of components would also potentially allow us to generate documentation programmatically based off types.

Details of the refactor 🔍

The refactors should introduce the least amount of runtime changes possible — ideally none. All JS files should be refactored to TypeScript.

You can also refer to the TypeScript migration guide in the @wordpress/components package guidelines., and reference an existing component like ItemGroup or ToolsPanel.

Next up

Once the refactor to TypeScript is complete, we'll be able to:

List ⚒️

The components in the exclude list of our tsconfig are particularly high priority.

Click to reveal the dependency graph ![dependencygraph](https://user-images.githubusercontent.com/1083581/150875105-2f977303-ec51-4b45-aeae-e49d0c3418b8.svg)
rajnish93 commented 2 years ago

Hello everyone :wave: I want to work on this issue. Can anyone tell me if I need to convert all js file to typescript. (example in DropdownMenu have index.js , index.native.js, test folder and stories folder) Do I need to change only index.js to typescript or all files.

ciampo commented 2 years ago

Hello everyone 👋 I want to work on this issue. Can anyone tell me if I need to convert all js file to typescript. (example in DropdownMenu have index.js , index.native.js, test folder and stories folder) Do I need to change only index.js to typescript or all files.

Hey @rajnish93 , thank you for your interest! I would ask you to wait before contributing to this issue, as I'm going to update the list of components that need refactoring. Some of the refactors are quite complex and it's probably better if we champion a few of them first to understand any caveat along the way.

We'd still love to get your help further down the line though, if that's ok!

ciampo commented 2 years ago

Alright, I've edited this issue's description and added all components/folders in the package.

There's still a couple of things of which I’m not sure yet:

  1. so far, even with fully-typed components, we left a few files still in JS — in particular unit tests, storybook examples, and react native files. Is there a particular reason for each of those?
  2. Where to draw the line in the refactor? On one hand, as we refactor, it would good to hook components to the context system, and therefore use the WordPressComponent type. On the other hand, we may want to keep the refactor as lean as possible and only aim changing file extension, fixing TS errors, and (maybe) adding a types.ts file. We should also consider that certain components are still written in class form — in that case, should we first refactor them to functional components in a separate PR preceding the TypeScript refactor?

Would love to hear folks' opinions on this (cc @mirka @diegohaz @sarayourfriend )

sarayourfriend commented 2 years ago

so far, even with fully-typed components, we left a few files still in JS — in particular unit tests, storybook examples, and react native files. Is there a particular reason for each of those?

From a "value" perspective I don't think there's a compelling reason to spend contributor time on updating unit tests and storybook examples to use TypeScript.

React native is a separate beast altogether. I'm not sure what it would take to convert those to TypeScript or if it's even possible with the current tooling.

Where to draw the line in the refactor? On one hand, as we refactor, it would good to hook components to the context system, and therefore use the WordPressComponent type. On the other hand, we may want to keep the refactor as lean as possible and only aim changing file extension, fixing TS errors, and (maybe) adding a types.ts file. We should also consider that certain components are still written in class form — in that case, should we first refactor them to functional components in a separate PR preceding the TypeScript refactor?

From a perspective of what will benefit the consumers of the components package the most, just adding types is a big win. The rest of the things help bring the components package inline with itself (context, Emotion, hooks, etc) but don't necessarily have a big effect on the consumers of the package. While converting to TypeScript will increase the maintainability of the package, it probably has the greatest effect on consumers of it. Given it will be the largest most widely used package to be typed in Gutenberg, it will also serve to evangelize the benefits of TypeScript to other contributors, many of whom may still have doubts that TypeScript is a valuable tool to learn in addition to everything else they've already been asked to learn to be able to contribute to Gutenberg (JavaScript, React (twice over now given the transition to hooks happened during Gutenberg's lifetime), etc).

Remember, we don't refactor just because we can. In the spirit of this, we should continue only converting to TypeScript (instead of just annotating with JSDoc) when necessary. This was the agreement made in the Make post that added TypeScript to Gutenberg (see When to use TypeScript for existing code section).

ciampo commented 2 years ago

From a "value" perspective I don't think there's a compelling reason to spend contributor time on updating unit tests and storybook examples to use TypeScript.

I see your point, although I liked the idea that a component would have all of its files in TypeScript, rather than a mix of TypeScript and JavaScript. But definitely not a priority when considering which order we should tackle components.

React native is a separate beast altogether. I'm not sure what it would take to convert those to TypeScript or if it's even possible with the current tooling.

I'll be seeking more advice from mobile folks on this matter specifically and come back with updates

Remember, we don't refactor just because we can. In the spirit of this, we should continue only converting to TypeScript (instead of just annotating with JSDoc) when necessary.

Thank you for the link! I would argue that a refactor to TypeScript has become, for this package, quite a necessity. As you explained too, it will bring benefits to the consumers of the package by adding first-party types, allowing us to deprecate (and stop maintaining) the DefinitelyTyped ones

Another added benefit that you mentioned is maintainability: the package is currently very fragmented in terms of technology:

Aligning the components in the package on the same tech stack as much as possible would make it easier for devs to maintain the package and for new folks to contribute to it.

Having said that, in the context of this TypeScript refactor, I think it makes the most sense to keep the refactor as "lean" as possible — i.e. adding the necessary types and refactoring the code without (theoretically) introducing any runtime changes. We can only iterate later on in case we want to introduce more changes to each component when necessary.

sarayourfriend commented 2 years ago

Having said that, in the context of this TypeScript refactor, I think it makes the most sense to keep the refactor as "lean" as possible — i.e. adding the necessary types and refactoring the code without (theoretically) introducing any runtime changes. We can only iterate later on in case we want to introduce more changes to each component when necessary.

Yes! I think this is the clearest path forward for this issue, otherwise the scope is far too large!

dcalhoun commented 2 years ago

so far, even with fully-typed components, we left a few files still in JS — in particular unit tests, storybook examples, and react native files. Is there a particular reason for each of those?

React native is a separate beast altogether. I'm not sure what it would take to convert those to TypeScript or if it's even possible with the current tooling.

Hi. 👋🏻 I spoke briefly with @ciampo about native modules in a chat, but I wanted to share thoughts here as well. Thanks for the ping on this topic! 🙇🏻

TL;DR: Native modules can import shared/web TypeScript modules successfully, both the ones that exist today and any modules converted in the future. Native modules themselves cannot be easily converted to TypeScript themselves currently due to https://github.com/microsoft/TypeScript/issues/21926.

I do believe the native mobile Gutenberg contributors are interested in typed JavaScript for the project. The project would likely gain a lot of benefit from static analysis and code completion. Also, leveraging TypeScript with React Native is fairly common in the community at large.

That said, I researched the possibility of using TypeScript for native modules in this repository mid last year. Unfortunately, it does not appear possible due to https://github.com/microsoft/TypeScript/issues/21926, which outlines TypeScript tooling's lack of support for platform-specific files.

Current Status of TypeScript for Native Gutenberg Modules

While a native module can technically be migrated to a TypeScript file, there is likely little value in a native module TypeScript file if (1) there is no way to enforce adherence to the native TypeScript module interface and (2) its interface cannot diverge from the sibling web TypeScript module.

So, with all of that, my personal opinion is that it is likely not worth refactoring native modules to TypeScript until some of these shortcomings in TypeScript's tooling are addressed, namely, https://github.com/microsoft/TypeScript/issues/21926 and https://github.com/microsoft/TypeScript/issues/420.

mchowning commented 2 years ago

🔴 Native modules cannot be statically typed checked via CI or editors as there is no way to configure TypeScript to load https://github.com/microsoft/TypeScript/issues/21926 or https://github.com/microsoft/TypeScript/issues/8328#issuecomment-219254627.

It looks like once https://github.com/microsoft/TypeScript/pull/48189 gets released we'll be able to configure TypeScript to load the .native files. 😄

ciampo commented 2 years ago

Update:

a first version of the TypeScript refactoring guidelines has been added to the @wordpress/components package guidelines.

From now on, we will be starting to accept help on this task — please follow the guidelines, open a draft PR and either post about it in this issue, or ping me and/or @mirka directly in the PR.

Thank you!

torounit commented 2 years ago

@ciampo @mirka

Rewrote to Typescript.

https://github.com/WordPress/gutenberg/pull/41216

t-hamano commented 2 years ago

I have marked Tooltip component as a blocker because refactoring is being considered in #42753.

kienstra commented 1 year ago

Hope it's OK I added a link to my friend Mike's ToolbarButton PR to this issue's top comment.

Just wanted to make sure there's no duplication of work.

margolisj commented 1 year ago

Looks like FooterMessageControl is native only at this point?

ciampo commented 1 year ago

Looks like FooterMessageControl is native only at this point?

Good point, I'm going to remove it from the list.

mirka commented 9 months ago

I think we can close this tracking issue now that we are practically done! At the component level, we only have the migration to CustomSelectControl v2 left, which is progressing and tracked separately (#55023).

Great work everyone 🎉