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: add TypeScript typings #410

Merged
merged 7 commits into from
Nov 11, 2020
Merged

feat: add TypeScript typings #410

merged 7 commits into from
Nov 11, 2020

Conversation

AndrewLeedham
Copy link
Contributor

Issue #, if available:

Description of changes:

  • Added types/index.d.ts which provides typings for style-dictionary when used in a TypeScript environment.
  • Added types field to the package.json so TypeScript knowns where the typings are.
  • Added types/index.d.ts to the files field in the package.json so it is published as part of the npm module.
  • Added typescript + dtslint + tslint configurations to lint the added types.

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

types/index.d.ts Outdated
declare namespace StyleDictionary {
interface Property {
value: string;
[attribute: string]: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've been away from typescript for awhile. I think that one goal is to avoid the use of 'any'.

Is it possible that this should be "string"? Or does it make sense in this context because this is how SD is structured?

If you have pointers to the logic here would be helpful for me to validate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is best practise to avoid any, however I used it here as the config JSON can contain any additional attribute. Thinking about this a bit more perhaps it would be better scoped to the types valid in JSON?
Perhaps something like this:

type JSONValue = null | string | JSONValue[] | JSONObject;
interface JSONObject {
    [key: string]: JSONValue;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also looking at this https://github.com/AndrewLeedham/style-dictionary/blob/master/lib/transform/propertySetup.js#L35 I think the internal type I called Prop should also have this behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JSONObject idea is not feasible. Unfortunately because other properties exist on the interfaces, an index signature would have to include non-valid props as well.

The built in JSON.parse function returns any so I think we will have to use any here.

@chazzmoney
Copy link
Collaborator

Hey, this looks pretty good. I'm a bit rusty on my TS, can you help remind me on that one comment?

Copy link
Collaborator

@chazzmoney chazzmoney left a comment

Choose a reason for hiding this comment

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

LGTM

@chazzmoney
Copy link
Collaborator

Going to delay merging for now. We would be adding tests to everyones systems, even those who don't use typescript.

We want to play with things a bit to make sure we don't impact those folks.

@hermanc-fda
Copy link

Have there been any other volunteers who've confirmed they've tried using this branch already? Thx

@AndrewLeedham AndrewLeedham requested a review from chazzmoney June 12, 2020 16:23
@chazzmoney
Copy link
Collaborator

Hi Andrew - sorry about the delay here.

I'm relooking at this, and I've got a couple concerns that if we can resolve, we can merge this.

  1. Is there a way we can make this optional so that those working without typescript will still be able to contribute - but those that have it still get the benefits? Seems like tying in the tsd would make it required.
  2. I've been away awhile, but tsd looks deprecated, as does its successor typings. Maybe this is just standard in the ts community?

@AndrewLeedham
Copy link
Contributor Author

  1. Is there a way we can make this optional so that those working without typescript will still be able to contribute - but those that have it still get the benefits? Seems like tying in the tsd would make it required.

tsd is only checking the typings themselves work as expected. If the JS API changes that won't throw any errors, only if the index.d.ts file changes. Ideally though if the JS API changes the typings will want updating otherwise they will be out of sync.

  1. I've been away awhile, but tsd looks deprecated, as does its successor typings. Maybe this is just standard in the ts community?

tsd is not deprecated to my knowledge it was updated a few months ago, it is still recommended on the official docs https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html#tips and by the Microsoft linting plugin used in DefinitielyTyped: https://github.com/microsoft/dtslint#just-looking-for-expecttype-and-expecterror

@didoo
Copy link
Contributor

didoo commented Oct 13, 2020

@luke-john FYI

@luke-john
Copy link
Contributor

Nice work 💖 💯 , thanks.

I've just tested this on our usage of style-dictionary at Bumble and it works beautifully. Will mean we can get rid of a lot of anys and a custom definition we'd written for style-dictionary.

The only change we'll need to make use of these, is to return a boolean rather than a falsy value from the matcher in registerTransform.

    matcher: function (prop) {
-        return prop.type === 'time' && prop.value.match(/^[\d.]+$/);
+        return prop.type === 'time' && !!prop.value.match(/^[\d.]+$/);
    },

The documentation does state that the matcher should return false though, so makes sense to have the types be strict about this 👍 https://amzn.github.io/style-dictionary/#/api?id=registertransform

@dbanksdesign dbanksdesign changed the base branch from master to 3.0 November 11, 2020 00:17
@chazzmoney
Copy link
Collaborator

LGTM :shipit:

@chazzmoney chazzmoney merged commit a8bb832 into amzn:3.0 Nov 11, 2020
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.

Provide TypeScript typings
6 participants