amzn / style-dictionary

A build system for creating cross-platform styles.
https://styledictionary.com
Apache License 2.0
3.94k stars 559 forks source link

chore(typescript): migrating to typescript #1248

Closed dbanksdesign closed 4 months ago

dbanksdesign commented 5 months ago

Issue #, if available:

Description of changes:

Initially found a bug with the DesignTokens type in types/DesignToken.d.ts when working on the docs:

CleanShot 2024-06-18 at 22 12 47@2x

TypeScript declaration files are not really type-safe because the TypeScript compiler isn't really being run on them. So I went down a rabbit hole of just migrating everything to TypeScript so that we don't inadvertently ship TS issues.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jorenbroekema commented 5 months ago

TypeScript declaration files are not really type-safe because the TypeScript compiler isn't really being run on them

This feels like improper usage or a bug or something, can't we fix this alone?

Added rollup to generate esm, cjs, and types as a build step

I would prefer to only publish ESM. CJS is a legacy format and has a lot of drawbacks:

There's more... but generally speaking CJS vs ESM is causing fragmentation in the JS ecosystem and people need to start pushing stuff towards ESM-only for it to get better, worst case scenario folks can always dynamically import ESM inside CJS files so it's not like we're alienating CJS users.

With regards to moving to TS files: I think the entire point of using JS files rather than TS files is so you don't have a build-step, no compilation process which you then need to also handle in your other tools, linting, testing, demoing/docs etc. TS files never work without first requiring you to set up a build process or plugin to make it precompile or compile on demand. Slows stuff down as well.

I think this PR demonstrates this nicely in the way the tests are ran now, we're currently running them from the dist folder which means in order to run your tests you have to recompile the TS files, which significantly worsens the TDD flow..

JSDocs annotations are the drawback, they're subjectively inferior DX while authoring, but .d.ts files to store your interfaces helps with this, and I find this tradeoff to be more than worth it.

Basically, every time in the last 2 years that I've switched over a project from JS files w JSDocs annotations for type safety to TS files, I've thoroughly regretted the move, the most recent one being the sd-transforms package.

Here's some reading material on this topic that I find helpful when explaining this perspective: https://dev.to/thepassle/using-typescript-without-compilation-3ko4 https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#supported-jsdoc https://medium.com/@trukrs/type-safe-javascript-with-jsdoc-7a2a63209b76 (kind of old though, doesn't highlight importing types from .d.ts files)

If the only issue is that TSC didn't properly check our .d.ts files for issues, I would much prefer to just fix that issue without going down this route.

dbanksdesign commented 5 months ago

I think this PR demonstrates this nicely in the way the tests are ran now, we're currently running them from the dist folder which means in order to run your tests you have to recompile the TS files, which significantly worsens the TDD flow..

I put a comment that this would actually not be how the tests should run, but I just didn't have time to port the test files to TS as well. If we port the test files to TS then we would not have to import dist files to test them, we would just import the source TS files.

Basically, every time in the last 2 years that I've switched over a project from JS files w JSDocs annotations for type safety to TS files, I've thoroughly regretted the move, the most recent one being the sd-transforms package.

I'm curious what troubles you have had, my experience in the past as been the exact opposite.

For ESM v CJS, I totally agree CJS has many issues, but at the same time it is not that hard to support with something like Rollup. In other projects I don't even think about ESM v CJS v TS anymore because the build step takes care of that pretty easily, and the build step takes less than a second.

One issue I have with the current setup is that our build command creates all the .d.ts files in the source code so I can't really test how end-users would interact with the package because if I build it then I need to stash all those changes before committing. Either way, we still have a build step, but the current approach is more cumbersome to deal with if I want to test importing style dictionary from another npm package.

I'm open to not migrating to typescript completely, my main concern is to not treat our typescript customers as second-class consumers anymore (which to my fault they have been so far). The main concerns I have:

  1. Ensuring type-safety and validity in an automated way (through a build step, a test step, whatever)
  2. Ensuring the developer experience (for both JS and TS devs) the best we can possibly make it
  3. To the second point, we should be able to test what an end-user would experience. This is why I'm also interested in moving to a mono-repo, so that we can consume style-dictionary in examples/docs as an end-user would.

One example friction point, which could be solved with the current architecture maybe?, is actually navigating to the types in VSCode. The first clip is the current way with JSDoc + declarations:

CleanShot 2024-06-20 at 10 41 20

And this is with the PR:

CleanShot 2024-06-20 at 10 42 08

jorenbroekema commented 5 months ago

I'm curious what troubles you have had, my experience in the past as been the exact opposite.

It's a long list and I don't remember everything from when I last ran into issues but here's a subset:

I don't even think about ESM v CJS v TS anymore because the build step takes care of that pretty easily

You'd be surprised about the amount of ESM v CJS interoperability issues that exist which bundlers do not take care of, at least not correctly or without configuring it to your need. Fortunately I haven't ran into them all too much for style-dictionary in particular (because the issues are somewhat niche), but when you do, it is an absolute nightmare to figure out why the CJS version is misbehaving while the ESM one works fine or vice versa. The amount of configuration here gives a rough idea: https://www.npmjs.com/package/@rollup/plugin-commonjs#options . And there isn't really a downside to forcing people to use ESM when you can still import ESM packages inside CJS modules just fine. Lastly, third party module tampering being an inherent security flaw of CJS modules really brings it over the line for me (although less of a concern in this package, since it's not crypto/security related).

I can't really test how end-users would interact with the package because if I build it then I need to stash all those changes before committing

Yeah that's generally the tactic, run build, npm link, then git clean -f to ditch the new untracked files. With TS files you don't have to build, can just link, but that only applies to other projects that use TS files, if a project uses JS files then you need the same flow applies to test locally. I'll admit this is a minor drawback though.

my main concern is to not treat our typescript customers as second-class consumers anymore

Fully agree, the type issue you found with the local .d.ts files + skipLibCheck was a bit of a surprise to me too, but fixable (see my PR).

  1. This is already implemented, lint:types script just runs TSC without emitting anything, verifying the types.
  2. Yeah your screenshots show a slightly different DX because auto-generated .d.ts files tend to be more simplified than hand-written ones, so I think writing our types in .ts files instead of .d.ts and emitting the .d.ts, will really help here. JSDocs import also supports .ts files. This difference in DX was not something I noticed before. I also read that during one of the typescript team internal meetings they aligned on the idea that .d.ts files are always output and not really supposed to be handwritten like we're doing, so that also kinda supports the argument for putting the type interface in .ts files
  3. I'm open to moving to a monorepo but if it's just the examples then maybe it's not really worth it, monorepos definitely have drawbacks too wrt overhead. I'm also thinking that maybe we can integrate the examples better through the docs site / playgrounds + eject button, didn't really get around to diving deeper into that