chore(typescript): migrating to typescript #1248

dbanksdesign · 2024-06-18T22:55:34Z

Issue #, if available:

Description of changes:

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

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.

lib/ -> src/ and changed all .js files to .ts files
Moved types/ -> src/types
Updated the tsconfig
Added rollup to generate esm, cjs, and types as a build step

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

chore: formatting-only changes for JS/MD files and pkg json

chore: bump husky, change precommit hook, eslint scripts

Version Packages (prerelease)

…yFormatter

V3 fixes port

Co-authored-by: Ioannis Chrysostomakis <[email protected]>

Forward port of bugfix and grammar fix in v3

Version Packages (prerelease)

feat: add preprocessors

chore: remove es6 helpers and use actual ES6

Fix tests & slight API changes

Version Packages (prerelease)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* fix: support optional props border tokens * fix: handle timingFunction and fontFamily props in transforms

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* feat: allow not throwing on broken refs * chore: add integration test broken refs + correct refs * fix: revert throwOnBrokenReferences, respect silent verbosity

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* feat: recursively expand nested composite tokens * fix: expand object type check, maintain ref if possible * fix: handle multi-value object tokens like shadows --------- Co-authored-by: Abel van Beek <[email protected]> Co-authored-by: jorenbroekema <[email protected]>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

dbanksdesign · 2024-06-19T06:00:38Z

__tests__/__setup.js

@@ -4,7 +4,7 @@ import { dirname } from 'path-unified';
 import { fs } from 'style-dictionary/fs';
 import { chaiWtrSnapshot } from '../snapshot-plugin/chai-wtr-snapshot.js';
 import { fixDate } from './__helpers.js';
-import { writeZIP } from '../lib/utils/convertToDTCG.js';
+import { writeZIP } from '../dist/esm/utils/convertToDTCG.mjs';


Eventually I would like to move the tests to TS as well and import source files rather than dist files, but we can save that for a separate PR

dbanksdesign · 2024-06-19T06:02:00Z

__integration__/android.test.js

@@ -13,7 +13,7 @@
 import { expect } from 'chai';
 import StyleDictionary from 'style-dictionary';
 import { fs } from 'style-dictionary/fs';
-import { resolve } from '../lib/resolve.js';
+import { resolve } from '../dist/esm/resolve.mjs';


Updated the test scripts to run a build first, so these files should exist

dbanksdesign · 2024-06-19T06:03:32Z

package.json

    },
-    "./utils": "./lib/utils/index.js",
-    "./types": "./types/index.d.ts"


Types are now just exported out of index

dbanksdesign · 2024-06-19T06:04:35Z

src/Register.ts

-    const ctor = /** @type {typeof Register} */ (this.constructor);
-    return deepmerge(ctor.hooks, this._hooks ?? {});
+  get hooks(): Required<Hooks> {
+    return deepmerge((this.constructor as any).hooks, this._hooks ?? {});


microsoft/TypeScript#3841

Yeah this issue is a nuisance but I don't see how typecasting to any is better than typecasting to typeof Register which is actually the proper cast and what it should be implicitly typed as if that TS issue were solved.

dbanksdesign · 2024-06-19T06:10:38Z

src/common/formatHelpers/index.ts

-export { default as minifyDictionary } from './minifyDictionary.js';
-export { default as setSwiftFileProperties } from './setSwiftFileProperties.js';
-export { default as setComposeObjectProperties } from './setComposeObjectProperties.js';
+export { default as createPropertyFormatter } from './createPropertyFormatter';


For future: export named functions and don't have to do export { default as _ }

jorenbroekema · 2024-06-19T11:14:50Z

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:

Module-tampering, vs ESM which is read-only
Interoperability issues that bundlers have an infamously hard time handling (mostly around imports/exports patterns)
Overhead in our build process, we have to add a bundler just to dual publish...

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.

jorenbroekema · 2024-06-19T11:17:08Z

package.json

+  "main": "./dist/index.js",
+  "module": "./dist/esm/index.mjs",


These are legacy fields and should be avoided. Package entrypoints has widespread support across all modern JS tooling and locks down public from private APIs which helps with reliability and prevents issues where people will complain that you broke some private API that they were relying upon

dbanksdesign · 2024-06-20T17:55:54Z

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:

Ensuring type-safety and validity in an automated way (through a build step, a test step, whatever)
Ensuring the developer experience (for both JS and TS devs) the best we can possibly make it
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:

And this is with the PR:

jorenbroekema · 2024-06-20T22:03:53Z

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:

Typescript does breaking changes in patches/minors (almost every TS change is technically potentially breaking so I don't blame them) and when this causes breakages in the types, you are now blocked from using the library until you fix it, because the TS files will not compile if there are type issues. Broken types in JS files are just in the comments, they always run fine even if broken.
Experimentation and debugging is a lot easier if you don't have to worry about types being temporarily broken, if I just want to see what removing a property does to the tests, I'd first have to fix dozens of type issues because of removing the property, I'm not interested in types being correct at that point, I just want to see the behavior.
All your tools need TS plugins of some kind, whether that's ts-mocha, rollup-plugin-typescript to get the Web Test Runner working, a TS plugin for ESLint, your storybook, docs site (happens to already work out of the box for astro but you get my point), this is tooling overhead and just makes things harder to work with, all of these tools don't need perfect types to do their jobs but if the files are TS then they do...
You have to deal with source maps to cover the fact that what the library consumer gets is not 1:1 with the source code. This makes it harder to debug the libraries you use and contribute fixes, because you have to translate your local monkey-patch fix to a proper fix in the source code. (this is kind of a minor point though)
Typescript isn't very fast, having the mandatory compilation step, even if it's only 2 seconds, means those 2 seconds happen for every tool you're using, and especially in watch-mode it can lead to very sluggish feedback from changing 1 line of code and saving to seeing the test output (fail/succeed)

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).

This is already implemented, lint:types script just runs TSC without emitting anything, verifying the types.
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
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

jorenbroekema and others added 30 commits September 15, 2023 16:55

chore: formatting-only changes for JS/MD files and pkg json

4140de9

Merge pull request #1013 from amzn/formatting

c5a9592

chore: formatting-only changes for JS/MD files and pkg json

chore: formatting createPropertyFormatter test/code

d043139

chore: add verify & release workflows (#1018)

7d2310c

BREAKING: make style-dictionary ESM-only and browser-compatible (#1014)

dcbe2fb

chore: bump husky, change precommit hook, eslint scripts

a45ac2f

Merge pull request #1028 from amzn/fix-husky-eslint

4d82091

chore: bump husky, change precommit hook, eslint scripts

chore: customize commit message changesets workflow

e43b147

chore: release (prerelease)

016ac6b

Merge pull request #1029 from amzn/changeset-release/v4

dab4161

Version Packages (prerelease)

fix: allow outputReferences to work on non-string values

24d41c3

feat: commentStyle and commentPosition improvements for createPropert…

c1dd5ec

…yFormatter

Merge pull request #1030 from amzn/v3-fixes-port

ddafafe

V3 fixes port

fix: allow falsy token values for outputReferences, e.g. 0

6fb81ad

chore: fix grammar issue in docs

6495951

Co-authored-by: Ioannis Chrysostomakis <[email protected]>

Merge pull request #1035 from amzn/port-ref-falsy

39c0c97

Forward port of bugfix and grammar fix in v3

chore: release (prerelease)

6c0ac72

Merge pull request #1031 from amzn/changeset-release/v4

0d3531c

Version Packages (prerelease)

chore: fix unit tests

1e19133

fix: logging to respect log level option, fix integration tests

984a71e

fix: add commentPosition to FormatHelpers.LineFormatting type interface

fd8c3b5

chore: add changesets

a4542f4

chore: remove es6 helpers and use actual ES6

3a380a6

feat: add preprocessors

cedf8a0

Merge pull request #1043 from amzn/preprocessors

82e4975

feat: add preprocessors

Merge pull request #1048 from amzn/es6-remove

e2493fb

chore: remove es6 helpers and use actual ES6

Merge pull request #1045 from amzn/fix-tests

269c077

Fix tests & slight API changes

chore: release (prerelease)

2b62332

Merge pull request #1050 from amzn/changeset-release/v4

efbfeee

Version Packages (prerelease)

chore: fix inject-version script

3591f50

jorenbroekema and others added 18 commits May 30, 2024 17:51

fix: allow register hooks override existing (#1223)

2335f13

feat: pass dictionary options to preprocessors (#1224)

3f09277

feat: pass ref path in getReferences util output (#1226)

6e226aa

chore: add missing changeset for SD format platform methods (#1227)

af5cc94

chore: release (prerelease) (#1225)

04c3377

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

timingFunction and fontFamily props, optional border props (#1229)

261a2cb

* fix: support optional props border tokens * fix: handle timingFunction and fontFamily props in transforms

chore: release (prerelease) (#1230)

6560456

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

fix: expand on platform level to adjust path prop (#1231)

ba03ee9

chore: release (prerelease) (#1232)

344cf66

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

fix: fix CLI, update basic example, fix android templates DTCG (#1235)

c06661d

chore: release (prerelease) (#1236)

1b8bdff

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

feat: allow not throwing on broken refs (#1240)

39f0220

* feat: allow not throwing on broken refs * chore: add integration test broken refs + correct refs * fix: revert throwOnBrokenReferences, respect silent verbosity

chore: release (prerelease) (#1242)

fdb308a

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

chore: release (prerelease) (#1245)

17985e5

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

chore(typescript): upgrading to typescript

c73ec7b

chore: fixing tests

d82a3fc

chore: updating scripts

a099636

dbanksdesign changed the title ~~chore(typescript): upgrading to typescript~~ chore(typescript): migrating to typescript Jun 19, 2024

dbanksdesign marked this pull request as ready for review June 19, 2024 05:39

dbanksdesign requested a review from a team as a code owner June 19, 2024 05:39

chore: fix fs

d64b8fe

dbanksdesign commented Jun 19, 2024

View reviewed changes

chore: fixes

4b50ddc

jorenbroekema reviewed Jun 19, 2024

View reviewed changes

jorenbroekema force-pushed the v4 branch from 7929e70 to 2253a56 Compare June 28, 2024 13:23

Base automatically changed from v4 to main June 28, 2024 23:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(typescript): migrating to typescript #1248

chore(typescript): migrating to typescript #1248

dbanksdesign commented Jun 18, 2024 •

edited

Loading

dbanksdesign Jun 19, 2024

dbanksdesign Jun 19, 2024

dbanksdesign Jun 19, 2024

dbanksdesign Jun 19, 2024

jorenbroekema Jun 19, 2024

dbanksdesign Jun 19, 2024

jorenbroekema commented Jun 19, 2024

jorenbroekema Jun 19, 2024

dbanksdesign commented Jun 20, 2024

jorenbroekema commented Jun 20, 2024 •

edited

Loading

chore(typescript): migrating to typescript #1248

Are you sure you want to change the base?

chore(typescript): migrating to typescript #1248

Conversation

dbanksdesign commented Jun 18, 2024 • edited Loading

dbanksdesign Jun 19, 2024

Choose a reason for hiding this comment

dbanksdesign Jun 19, 2024

Choose a reason for hiding this comment

dbanksdesign Jun 19, 2024

Choose a reason for hiding this comment

dbanksdesign Jun 19, 2024

Choose a reason for hiding this comment

jorenbroekema Jun 19, 2024

Choose a reason for hiding this comment

dbanksdesign Jun 19, 2024

Choose a reason for hiding this comment

jorenbroekema commented Jun 19, 2024

jorenbroekema Jun 19, 2024

Choose a reason for hiding this comment

dbanksdesign commented Jun 20, 2024

jorenbroekema commented Jun 20, 2024 • edited Loading

dbanksdesign commented Jun 18, 2024 •

edited

Loading

jorenbroekema commented Jun 20, 2024 •

edited

Loading