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

feat(typescript/combine-json): add .ts file processing if runtime sup… #1392

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Nov 15, 2024

As described in #1391 many runtimes implement support for executing .ts files without any need for transpilation.

It seems the only problem is that style-dictionary tries to parse .ts files with JSON5.parse rather than just importing the file like the regular .js files.

I've tested the changes with:

  • deno v2.0.0
  • node --experimental-strip-types v22.9.0
  • bun v1.1.29

closes #1391

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

@hsjobeki hsjobeki requested a review from a team as a code owner November 15, 2024 10:05
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1392.d16eby4ekpss5y.amplifyapp.com

Copy link
Collaborator

@jorenbroekema jorenbroekema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few notes, but overall I like adding support for "native" TS token files.

  • can you add a changeset with npx changeset locally, probably a minor bump for this
  • I wonder if it would be possible to create a test for this feature that we only run in the CI workflow for node version which supports the strip types experimental flag, and run Node in the CI with that flag. The test would have to run conditionally based on whether the flag is active, maybe with some kind of environment variable

lib/utils/combineJSON.js Outdated Show resolved Hide resolved
lib/utils/combineJSON.js Outdated Show resolved Hide resolved
@hsjobeki
Copy link
Contributor Author

hsjobeki commented Nov 15, 2024

For reference here is the example log output for running node without --experimental-strip-types

Could not import TypeScript file: ./tokens/token.ts

Executing typescript files during runtime is only possible via
- 'node >= 22.6.0' with '--experimental-strip-types'

Alternatively by using 'deno' or 'bun'

If you are not able to satisfy the above requirements, consider transpiling the TypeScript file to plain JavaScript before running the Style Dictionary build process.

node:internal/modules/esm/get_format:218
  throw new ERR_UNKNOWN_FILE_EXTENSION(ext, filepath);
        ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Failed to load or parse JSON or JS Object: Failed to load or parse JSON or JS Object: Unknown file extension ".ts" for ./tokens/token.ts
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:218:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:244:36)
    at defaultLoad (node:internal/modules/esm/load:122:22)
    at async ModuleLoader.load (node:internal/modules/esm/loader:570:7)
    at async ModuleLoader.moduleProvider (node:internal/modules/esm/loader:443:45)
    at async ModuleJob._link (node:internal/modules/esm/module_job:106:19) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}
Node.js v22.9.0

jorenbroekema
jorenbroekema previously approved these changes Nov 25, 2024
jorenbroekema
jorenbroekema previously approved these changes Nov 25, 2024
Copy link
Collaborator

@jorenbroekema jorenbroekema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started adding some more tests and realised we would probably want to support TS configs as well, we had some duplicate code going on. So I extracted the file loading stuff into its own util, with its own tests. It's much cleaner now I think.

I also made it a minor bump since this new feature is pretty significant.

@dbanksdesign needs your review since I now co-authored this PR instead of only reviewing.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Dec 2, 2024

Any updates on this?
I am using my own fork currently. Would be nice if this makes it into the next release.

@jorenbroekema
Copy link
Collaborator

Rebased & fixed one failing test, just waiting now for approval by Amazon, imo this should be in next release indeed, but depends a bit on @dbanksdesign 's availability as for when that happens, we have quite a few other ready PRs that I also wanna include that need approval as well. 4.3.0 will be a big one :)

@jorenbroekema jorenbroekema merged commit ecf43c8 into amzn:main Dec 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Typescript token files support.
3 participants