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

Refactor @wordpress/components package to TypeScript #35744

Closed
andrewserong opened this issue Oct 19, 2021 · 15 comments
Closed

Refactor @wordpress/components package to TypeScript #35744

andrewserong opened this issue Oct 19, 2021 · 15 comments
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@andrewserong
Copy link
Contributor

andrewserong commented Oct 19, 2021 •
edited by mirka
Loading

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:

  • migrate all components to Emotion for styles
  • refactor folder structures, potentially split into hook+component
  • refactor components to use context
  • remove (or mark as deprecated) the third party types

List ⚒️

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

Click to reveal the dependency graph

dependencygraph
@andrewserong andrewserong added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 19, 2021
@andrewserong andrewserong mentioned this issue Oct 19, 2021
Closed
@gziolo gziolo added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Oct 24, 2021
@ciampo ciampo added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Nov 8, 2021
@ramonjd ramonjd mentioned this issue Nov 17, 2021
Closed
7 tasks
@rajnish93
Copy link

rajnish93 commented Jan 21, 2022 •
edited
Loading

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.

@ciampo ciampo changed the title Components: Refactor existing components to TypeScript and Emotion Components: Refactor existing components to TypeScript Jan 24, 2022
@ciampo
Copy link
Contributor

ciampo commented Jan 24, 2022

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 ciampo changed the title Components: Refactor existing components to TypeScript Refactor @wordpress/components package to TypeScript Jan 24, 2022
@ciampo ciampo removed the Good First Issue An issue that's suitable for someone looking to contribute for the first time label Jan 24, 2022
@ciampo
Copy link
Contributor

ciampo commented Jan 25, 2022

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
Copy link
Contributor

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
Copy link
Contributor

ciampo commented Jan 25, 2022

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:

  • some components are written in JavaScript, some in TypeScript
  • some components are styles in Sass, some in Emotion
  • some components use the context system, most don't

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
Copy link
Contributor

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
Copy link
Member

dcalhoun commented Jan 28, 2022 •
edited
Loading

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 microsoft/TypeScript#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 microsoft/TypeScript#21926, which outlines TypeScript tooling's lack of support for platform-specific files.

Current Status of TypeScript for Native Gutenberg Modules

  • 🟢 Native modules do successfully import and compile shared TypeScript modules, presumably due to Gutenberg’s existing TypeScript configuration and React Native’s default TypeScript support. E.g. MissingEdit imports Icon from @wordpress/components.
  • 🟢 Native modules do currently benefit from code completion of shared modules which include a single, unified interface for both web and native — this applies to both TypeScript and JSDoc definitions.
  • 🔴 Native modules are not statically typed checked via CI or editors due to Gutenberg’s existing TypeScript configuration excluding native files.
  • 🔴 Native modules cannot be statically typed checked via CI or editors as there is no way to configure TypeScript to load platform-specific files or enforce a common interface.
  • 🔴 Native modules cannot benefit from code completion from TypeScript and JSDoc definitions in native modules, e.g. index.native.[tsx|js]. The definitions from the sibling web module is always loaded, as there is no way to configure TypeScript to load platform-specific files.

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, microsoft/TypeScript#21926 and microsoft/TypeScript#420.

@rajnish93 rajnish93 mentioned this issue Jan 30, 2022
Closed
8 tasks
@ciampo ciampo mentioned this issue Feb 11, 2022
Closed
17 tasks
This was referenced Mar 9, 2022
Merged
Closed
Merged
@mirka mirka mentioned this issue Mar 15, 2022
Merged
@ciampo ciampo mentioned this issue Mar 16, 2022
Merged
@mchowning
Copy link
Contributor

mchowning commented Mar 30, 2022 •
edited
Loading

🔴 Native modules cannot be statically typed checked via CI or editors as there is no way to configure TypeScript to load microsoft/TypeScript#21926 or microsoft/TypeScript#8328 (comment).

It looks like once microsoft/TypeScript#48189 gets released we'll be able to configure TypeScript to load the .native files. 😄

@ciampo ciampo mentioned this issue Apr 7, 2022
Merged
@ciampo
Copy link
Contributor

ciampo commented Apr 25, 2022

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!

@walbo walbo mentioned this issue Apr 26, 2022
Merged
@ciampo ciampo mentioned this issue Apr 28, 2022
Merged
@mirka mirka mentioned this issue Apr 29, 2022
Merged
@flootr flootr mentioned this issue Mar 14, 2023
Merged
@mirka mirka mentioned this issue Mar 15, 2023
Merged
@chad1008 chad1008 mentioned this issue Mar 20, 2023
Merged
@margolisj margolisj mentioned this issue Mar 21, 2023
Merged
This was referenced Mar 21, 2023
Merged
Merged
@kienstra kienstra mentioned this issue Mar 27, 2023
Merged
@chad1008 chad1008 mentioned this issue Apr 28, 2023
Merged
@margolisj margolisj mentioned this issue May 2, 2023
Closed
@sunyatasattva sunyatasattva mentioned this issue Jun 8, 2023
Merged
This was referenced Jun 8, 2023
Merged
Closed
@mirka mirka mentioned this issue Jul 25, 2023
Closed
@bangank36 bangank36 mentioned this issue Jul 29, 2023
Merged
This was referenced Aug 28, 2023
Merged
Merged
Merged
@margolisj margolisj mentioned this issue Sep 8, 2023
Merged
@margolisj
Copy link
Contributor

Looks like FooterMessageControl is native only at this point?

This was referenced Sep 8, 2023
Merged
Merged
@ciampo
Copy link
Contributor

ciampo commented Sep 9, 2023

Looks like FooterMessageControl is native only at this point?

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

This was referenced Sep 30, 2023
Merged
Merged
@mirka
Copy link
Member

mirka commented Dec 20, 2023

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 🎉

@mirka mirka closed this as completed Dec 20, 2023
@mirka mirka mentioned this issue Dec 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
WordPress Components
Status: Done 🎉
Milestone
No milestone
Development

No branches or pull requests