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

[RFC] TypeScript migration #2479

Closed
vpicone opened this issue Apr 28, 2019 · 20 comments
Closed

[RFC] TypeScript migration #2479

vpicone opened this issue Apr 28, 2019 · 20 comments
Labels
package: react carbon-components-react status: inactive Will close if there's no further activity within a given time

Comments

@vpicone
Copy link
Contributor

vpicone commented Apr 28, 2019

Summary

TypeScript is a superset of Javascript – all Javascript is valid Typescript. However, once a project has typescript enabled, you can annotate your code to provide type hints to your users and other developers.

Just as shared styles between Carbon frameworks improves user experience, so to would shared behavior. Typesafety allows us to ship components that perform and behave as expected.

Related issues: #1296, #727, #1103, #580

Motivation

End users, product developers, as well as Carbon developers would all benefit from the addition of type safety to Carbon products

  • End Users – by shipping more resilient, bug free components end users will encounter fewer unexpected edge cases
  • Product developers – developers would be guided towards proper API usage and would be alerted to new or changed apis.

JS Example One

JS Example Two

  • Carbon developers and Typescript developers
  • Fewer Uncaught TypeError(Cannot read x of undefined or spelling errors means more time on real issues.
  • Fewer undefined refs and instance properties
  • Reduce reliance on null and undefined guards means more resiliant APIs
    – Alerted to misuse of props, state, third-party library, and dom errors while writing your code.

TS Example

Design

There are currently to main ways to compile typescript into javascript(the typescript compiler, and through babel. Since our project is already using Babel, switching to typescript would be unnecessary.

Namespaces and const enums are not supported by the babel compilation, however, this can be mitigated with compiler configuration.

As we look toward migrating codebases to a mono-repo, typesafety will offer a means to share types between projects and permit large scale refactors with confidence.

Drawbacks

  • Time spent onboarding carbon devs to typescript
    • I volunteer as tribute
  • Friction to carbon contributors new to the project
    • would empower them to make more meaningful changes
    • would reduce friction for folks onboarding to product teams

Alternatives

Non-Typescript

  • FlowType
    • call-site type checking is nice
    • Lots of projects switching to TS(Jest, Expo, Atlassian, Yarn.
    • Far fewer third party integrations 1/10th of the questions on SO
  • JSDoc
    • some bugs around object types
    • errors in jsdoc means no warnings/errors it just won't get annotated

TypeScript without migration

These solutions offer some of the benefits of TypeScript types with different drawbacks. While the core library would still be standard JavaScript maintaining types would be a manual effort.

The main issue with these strategies is that they require a much more manual effort up front and the types would be of lower quality than those generated natively from the project in situ.

Definitely-typed

  • Types published using the DefinitelyTyped repo https://github.com/DefinitelyTyped/DefinitelyTyped
  • Recommended solution from TS team for no TypeScript projects
  • Rely on community involvement
  • Issues keeping types in parity with new library features and changes
  • We don't benefit from the types internally

Internal .d.ts files

Adoption strategy

At no point will typescript migration stop or prevent work on the project. TypeScript can get out of the way when necessary (@ts-ignore and any or unknown types). It will never break builds unless we chose to enable that behavior.

While each of the Carbon javascript projects would benefit from the addition of types. I believe the most valuable library at this time would be carbon components react (CCR).

Since we already have some run-time type checking through prop-types, we already have a starting point. In addition, thanks to prop-types CCR developers are already at least slightly familiar with the constraints imposed by types.

Conversion path

A – Just rename everything to .ts and .tsx

  1. The first pass at converting the project will rename the files and add babel, storybook, and jest configuration to handle these new file types.

  2. The initial config will have the loosest possible config. Separate PR's will be made to turn on each of the strict-mode rules separately resolving issues as they come up.

  3. Any objective errors uncovered as a result of the conversion will be resolved as part of the same PR.

  4. Prop types will be defined at the same level of specificity as the prop-types. No unnecessary types added.

  5. Types can be shipped from day one

B – Leave the files as .js config enable allowJS and checkJS

  1. Recommended by TypeScript team for large projects using the TypeScript compiler (likely not us)
  2. All new code should be written as .ts
  3. Gradually migrate files to .ts
  4. Types can be shipped once all .js files are converted to TypeScript

Migration stories from others

  • hootsuite: Thoughts on migrating to TypeScript
  • clayallsop: Incrementally-migrating-javascript-to-typescript-565020e49c88 'Incrementally Migrating JavaScript to TypeScript
  • pleo: Migrating a Babel project to TypeScript
  • mstsreactconversionguide: TypeScript React Conversion Guide
  • entria: Incremental Migration to TypeScript on a Flowtype codebase
@asudoh
Copy link
Contributor

asudoh commented Apr 29, 2019

Thanks @vpicone for writing this up! For full TypeScript conversion I'd like careful observation on the learning curve of our developers/contributors. Though there are extra efforts to get .d.ts and .js in sync as you pointed out, I see .d.ts is a worthwhile alternative for the interim.

@vpicone
Copy link
Contributor Author

vpicone commented Apr 29, 2019

@asudoh the issues I see with that approach are:

  1. Maintaining the types would require manual effort as type generation tools are spotty at best. That manual effort would be concentrated on those intimately familiar with typescript as the types would be written from scratch. This also adds friction to new contributors if their changes would break types.
  2. We wouldn't benefit from the types internally leaving many of the benefits I listed out. Using the DefiniteyTyped repo is the recommended method for the typescript team.
  3. This approach is much more difficult to implement progressively. With a typescript migration, we can ship types when they are ready, not necessarily all at once.

The DefinitelyTyped types I published for @carbon/elements are a perfect example. After my initial commit, they've fallen out of sync very quickly.

For the learning curve, the typescript onboarding difficulty depends largely on the strictness of the rules, as just regular .js is still valid typescript. While there would be some learning for the 25 or so individuals who contribute to the library, the learning curve for the 5-10k devs who consume the library would be diminished.

Out of sync types could cause more harm than good for end users and actually end up costing dev time in the end, rather than saving it.

@vpicone
Copy link
Contributor Author

vpicone commented Apr 29, 2019

That said I believe Material-UI is using internal .d.ts files, so could be a good example of that the hand written types method.

@asudoh
Copy link
Contributor

asudoh commented Apr 29, 2019

Wrt learning curve - Another problem I'd like careful look at is getting more people onboard to our development, to better serve our users for the various kind of problems in our backlog. Would like to see a good balance between solving TS problems vs. solving other problems.

@vpicone
Copy link
Contributor Author

vpicone commented Apr 29, 2019

With the rules being progressively more strict, solving TS problems would be solving JS problems as well as preventing future issues. For example, the modal trap just recently ended up causing even more problems than it solved. The undefined issue we shipped that broke the modal never would have been prevented with typescript. That issue remained broken for quite a bit and cost significant dev time for review. That time could have been spent on the backlog.

While we might be introducing friction for contributors unfamiliar with typescript, I think that types would allow contributors to work more confidently in complex components and more quickly orient themselves in the code base. It also allows us to be more confident that their changes aren’t breaking.

In my opinion, the types already exist, they’re just in our mental modal of the project. Typescript allows us to formalize them for new and existing contributors.

@emyarod
Copy link
Member

emyarod commented Apr 29, 2019

thanks @vpicone for drafting this RFC! I'm excited to potentially introduce TS into our project and have been looking to for a bit now. As @asudoh mentioned, we need to carefully weigh the cost and benefits to justify the investment into TS. That being said, I am all for this initiative. prop-types is good but still feels kind of lacking, and in my experience the self-documenting nature of strong static typing should ease the onboarding process for devs unfamiliar with the project (which should also make adoption smoother especially as our project grows). I also think this alleviates a lot of the pressure on the contributors (especially the maintainers) to code defensively. of course this is all anecdotal evidence from me, and I'm interested in hearing the perspectives from others

on a side note I agree that TS would have helped prevent a lot of the headaches that introducing the modal focus trap caused, but I think part of that may have stemmed from adding dependencies without fully vetting them, which is why this RFC and discussion is so crucial!

@joshblack
Copy link
Contributor

Couple of initial questions that I have when we think of TypeScript integration:

  • What happens if a language feature we are using (or want to use) is supported in babel but not in TypeScript?
  • Would we have to both type our props and offer prop types for teams not using TS? How would we keep these in sync?

@vpicone
Copy link
Contributor Author

vpicone commented Apr 29, 2019

@joshblack if the feature changes the syntax significantly, we'd need to use a babel macro until the language feature is supported by typescript (level 3+). Otherwise, all the standard plugins would work. As an example, object rest/spread wasn't typescript supported for quite some time and folks would include the babel plugin.

TS definitely isn't a drop in replacement for prop-types, especially for libraries that are consumed by JS projects like ours, since it disappears at run time. Typescript will throw an error if the prop-types and Prop types don't map:

proptypes

Also, there's a babel plugin to set prop-types from typescript types. If we wanted to automate the process of generating them.

@carbon-bot carbon-bot transferred this issue from carbon-design-system/carbon-components-react May 9, 2019
@carbon-bot
Copy link
Contributor

Hi there! 👋 If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: [email protected]. Thanks!

@stale
Copy link

stale bot commented Jun 8, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Jun 8, 2019
@vpicone
Copy link
Contributor Author

vpicone commented Jun 11, 2019

NOT STALE

@stale stale bot removed the status: inactive Will close if there's no further activity within a given time label Jun 11, 2019
@stale
Copy link

stale bot commented Jul 11, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Jul 11, 2019
@stale
Copy link

stale bot commented Jul 14, 2019

As there's been no activity since this issue was marked as stale, we are auto-closing it.

@stale stale bot closed this as completed Jul 14, 2019
@vpicone vpicone reopened this Aug 5, 2019
@stale stale bot removed the status: inactive Will close if there's no further activity within a given time label Aug 5, 2019
@JSoet
Copy link

JSoet commented Aug 5, 2019

Just wanted to mention my support for moving to typescript! Our team also uses typescript, and of course I would love having types when using carbon....

But also I think in the long run switching to typescript will improve the quality of the carbon code... Our team also struggled with the decision to decide whether or not to adopt typescript, whether it was worth the effort for our developers to deal with the extra overhead of learning typescript and typing everything, but overall I think everyone on our team is now happy that we did it.

Of course there's some times still when we are struggling with the typing even though the code itself works, and some times where there can be bugs that typescript didn't catch because the types are only as good as you make them... But it has allowed us to be a lot more confident in doing refactorings, and allowed us to confidently make some big changes and improvements to our codebase that we were usually too timid to do before...

@vpicone
Copy link
Contributor Author

vpicone commented Aug 5, 2019

Totally agree, even in partially implementing typescript I found some issues that I wouldn’t have detected otherwise. I feel like for a top level component library those sorts of issues should be air tight.

@spgill
Copy link

spgill commented Aug 6, 2019

Here to voice my support for TypeScript typings for Carbon.

I work on a team within IBM with a codebase written almost entirely in TypeScript. With the big push for us to adopt the new Carbon/IDL styles into our product, it's been a really hard decision for us to pull in the carbon-components-react package given its lack of TS support and what that would mean for our code integrity. In the interim we're going to have to rely on creating our own rudimentary typings to get by. Contributing our typings to definitely typed is an option, but it would take a back seat to our business priorities.

Inclusion of a full suite of TS typings into the official codebase (or through definitely typed) would be a huge boon to my team, and I'm sure several others that are looking to adopt Carbon.

@kalbert312
Copy link
Contributor

Moved my typings to a definitelytyped PR: DefinitelyTyped/DefinitelyTyped#37442

@stale
Copy link

stale bot commented Sep 6, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Sep 6, 2019
@stale
Copy link

stale bot commented Sep 9, 2019

As there's been no activity since this issue was marked as stale, we are auto-closing it.

@stale stale bot closed this as completed Sep 9, 2019
@michaeljota
Copy link

Do you have any updates on this? I think moving into TS will really improve the developer experience, including your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: react carbon-components-react status: inactive Will close if there's no further activity within a given time
Projects
None yet
Development

No branches or pull requests

9 participants