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

Adopting TypeScript #1715

Closed
swissspidy opened this issue May 11, 2020 · 4 comments
Closed

Adopting TypeScript #1715

swissspidy opened this issue May 11, 2020 · 4 comments
Labels
Type: Code Quality Things that need a refactor, rewrite or just some good old developer ❤️ Type: Infrastructure Changes impacting testing infrastructure or build tooling

Comments

@swissspidy
Copy link
Collaborator

swissspidy commented May 11, 2020

Feature Description

Possibly adopting TypeScript for our code base has been brought up a few times in the past.

A step in that direction was made at some point by switching our JSDoc parsing to use TypeScript, allowing for more expressive type documentation. See #2960.

That's a pretty common thing to do and is something Gutenberg has been doing as well, for example. Gutenberg even creates d.ts files based on the JSDoc annotations. That's neat because it improves DX without actually having to write TypeScript code. That path has its limits, however.

Actually authoring code in TypeScript is of course a whole different topic.

My proposal is not to just convert our hundreds of thousands of lines of code to TypeScript. However, TypeScript could be selectively used for some parts of the code base.

In particular, I think it would be really helpful for low-level code that does not require frequent maintenance. For instance:

  • Media-related code (Resource, createResource)
  • Code related to the story/elements data structure, which is the heart of the story editor after all
  • Small packages like tracking, i18n or url that change rarely but are used often

Some considerations off the top of my head:

  • Parsing TypeScript can be slower. We need to ensure that builds and tests are not significantly slower.
  • Not everyone on the team is familiar with TypeScript

Other benefits:

  • More alignment with Google-internal tooling.

Overview

Package Status Issue / PR
date #12224
dom #12218
i18n #12198
media #12127
moveable #12169
patterns #12200
react #12127
stickers #12204
transform #12202
tracking #12085 / #10783
units #12127
url #12212

Additional Context

Gutenberg went through a similar process. These posts are a great resource on that:

@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Type: Infrastructure Changes impacting testing infrastructure or build tooling Type: Code Quality Things that need a refactor, rewrite or just some good old developer ❤️ labels May 11, 2020
@jauyong jauyong added Pod:WPInfra and removed Type: Infrastructure Changes impacting testing infrastructure or build tooling labels May 15, 2020
@swissspidy swissspidy added Type: Infrastructure Changes impacting testing infrastructure or build tooling and removed Type: Enhancement New feature or improvement of an existing feature labels Jul 3, 2020
@dreamofabear dreamofabear added the P3 Nice to have label Sep 22, 2020
@swissspidy swissspidy added P4 and removed P3 Nice to have labels Feb 9, 2021
@BrittanyIRL
Copy link
Contributor

Thanks for posting your proof of concept here with the tracking package!

So, I'm not the most familiar with typescript in a prod environment, I use it sparingly in my own time and have taken a handful of classes on it. @littlemilkstudio and @samwhale are both well versed in it and worked with it before this project within Formidable at another client. That said, it feels like we're at the point where this is a really good idea to dig into more. Specifically, I think converting the story/element data structure and media resources (which you pointed out already) would be really helpful in moving to a shared state solution and then further down the road as we look into replacements for draft js. Having the story structure strongly typed so that the whole of it is defined before we make those moves would make the code stronger along the way, preventing weird edge case regressions. It's also going to help give context to devs who didn't write the base structure.

My thought is that we should come up with a list of qualifications for when to migrate something to typescript, this would give us a way to track what we need to update and then that list of qualifications can become an identifying pattern for when new code gets written too. Also, probably starting on the smaller, frequently used packages (as Pascal did with the POC tracking) would work well (not to just completely echo @swissspidy here).

I can safely say that pea pod would be really excited to work on this update.

@littlemilkstudio @samwhale ya'll wanna chime in with thoughts/ideas?

@maxyinger
Copy link
Contributor

In particular, I think it would be really helpful for low-level code that does not require frequent maintenance

Yup yup. this sounds good to me.

My only advice is that it's easy to import something written in TS into JS, but it's not so nice to import JS into TS. Downsides of importing JS into TS include:

  • Either opt out of a lot of the typing (which really minimizes the benefit of having something written in TS in the first place)
  • Or you retro-actively type out all the JS you're importing, which compromises scope and partial (or incremental) adoption

Any adoption strategy that is aware of this will probably fare well. The low-level code you mentioned seems like a good candidate here for these reasons

@swissspidy
Copy link
Collaborator Author

Any adoption strategy that is aware of this will probably fare well. The low-level code you mentioned seems like a good candidate here for these reasons

I recall previous discussions about starting with migrating leaf packages first, which would align with that 👍

So overall we're on the same page I think.

A list of qualifications sounds good to me, though this can probably wait until after a concerted effort to migrate the core data structure, which would be prio no. 1.

Speaking of, protocol buffers might be the best source of truth for that. It's what we typically use internally. We experimented with that previously in #1044. There are code generators such as https://github.com/stephenh/ts-proto to automatically create TS interfaces from .proto files (the default compiler only supports JS output).

@swissspidy
Copy link
Collaborator Author

Posting here for context, @merapi mentioned how the bug fixed by #11136 could have been prevented by making the useSnapping() hook strongly typed with making it return SnappableProps from Moveable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Code Quality Things that need a refactor, rewrite or just some good old developer ❤️ Type: Infrastructure Changes impacting testing infrastructure or build tooling
Projects
None yet
Development

No branches or pull requests

5 participants