[POC] Add TypeScript support and convert a couple of files

[eldarshamukhamedov]

Context

Hey folks, came across this library a few weeks back, and really liked the deep property merge approach! 🎉 Wanted to take a stab at adding TypeScript support, in case you were interested in fully moving to TS at some point. Let me know if you want to commit to fulling moving to TS, and I'll be glad to take on the dev work. 🙂

I just added the basic TS build support and converted a couple of the smaller files. One of my goals was for these changes to be completely transparent to the current users. For this reason, I decided to stick with using Babel for compilation (build artifacts before and after this PR are identical), and only use the TypeScript compiler (tsc) to generate type declaration files (.d.ts).

In this setup, Babel doesn't actually do any type checking; it just strip them out. Instead, you have to rely on a combination of your editor config and tsc to catch type errors (see the build:typescript:watch NPM script). Babel is also a bit faster than tsc, since it skips type checks, which make this setup a pretty decent dev experience.

That said, I think the ideal and simplest build setup is to drop Babel + core-js in favor of tsc with the right set of polyfills (See the --lib compiler flag).

Changes

• [x] Use tsc to generate type declaration files only
• [x] Use babel to transpile and strip out types
• [x] Add ESLint TS overrides
• [x] Preserve existing polyfills (core-js instead of TS libs)
• [x] Preserve build output (only .d.ts files added)
• [x] Add Two TS configs: i. tsconfig.json base for local type checking (includes test files, doesn't emit build artifacts) ii. tsconfig.build.json for generating type declarations (excludes test files, emits .d.ts files only)
• [x] Configure Jest to support TS files
• [x] Convert a couple of files and corresponding tests to TS

Open questions

• Is moving to TS something the NYT team willing to consider for this package?
• What are you thoughts on dropping Babel+Core.js in favor of plain TS?
• The decorator factory seems to support some sort of lazy initializer that's not standard. The core TS TypedMethodDecorator interface is missing the initializer properly that is used in makeClassMemberDecorator. I pretty much never use decorators (hooks, baby! 😅), so would need dig a bit to figure out how to type this properly. Is there a library that you guys use for lazy loading at NYT?

Next steps

• Resolve the open questions 🙂
• Compare generated types to the community @types/react-training types
• Convert the rest of the files and tests
• Possibly drop Babel
• Stay safe during the quarantine! ❤️

[tizmagik]

Hey this is awesome @eldarshamukhamedov thanks for putting this together! I'm very sorry it's taken me so long to get back to you on this PR, it's been quite the past several weeks at work as you could imagine 😬

For your questions:

Is moving to TS something the NYT team willing to consider for this package?

Yeap, definitely! We're starting to use TS more internally so I think it would be a welcome addition here as well.

What are you thoughts on dropping Babel+Core.js in favor of plain TS?

I would be fine with that, but I'd want to ensure that we don't break any existing functionality, if at all possible. I'm not sure what the differences between the legacy decorators babel transformer is compared to TS's decorators. I'm sure there are differences so I'd like to get a better handle on them and provide clear changelog directions and/or shim those differences if at all possible. Do you have more insight here?

The decorator factory seems to support some sort of lazy initializer that's not standard. The core TS TypedMethodDecorator interface is missing the initializer properly that is used in makeClassMemberDecorator. I pretty much never use decorators (hooks, baby! 😅), so would need dig a bit to figure out how to type this properly. Is there a library that you guys use for lazy loading at NYT?

We rely on the decorator syntax fairly heavily at NYT so it's not an option to break that functionality. That being said, I believe the e2e test suite in this project is exhaustive of those use cases, so if those still pass in the same way then I'd be more confident in that change. What are your thoughts?

--

Curious to get your take on the above, but in the mean time, I'm going to review this PR as soon as I have some time and post back with thoughts.

Thanks again for this, this is awesome! 🙏

[tizmagik]

Hey @eldarshamukhamedov -- wanted to bump this, are you still interested in landing this PR? If so, any thoughts on the questions above?

Thanks again for your contribution!

[jkoutavas]

Can we please have this PR approved+merged?

[tizmagik]

I'd be happy to, but it needs to be updated. Most notably, there's now a new <Track /> component returned in v8.1.0

Also happy to support full type declarations in a follow-on PR but I don't have the bandwidth to do that work myself at this time so I'd need someone to step up and commit to that work or at least start us off.

[eldarshamukhamedov]

Hey @tizmagik, apologies for radio silence on this one. IIRC I started looking into enabling TS support for the legacy decorator syntax this lib was using, but gave up 😓. It's possible to enable it, but since I don't use decorators, I lost motivation haha. Looking back at how this PR was set up, I think the best option is actually to either convert the code base to TS, or maintain a separate TS types.d.ts file manually.

The API surface of the library is pretty small, so a manual types file might be manageable, but of course that depends on whether NYT has an internal use for TS type defs for this package.

For a full JS->TS migration, I ended up taking a lot of queues from this library and wrote a hook in TS that has nearly identical functionality (although no decorator support 🙃). Here's the hook and package, in case that helps with a migration to TS for this library.