-
Notifications
You must be signed in to change notification settings - Fork 553
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
Add object handling for typescript/es6-declarations #718
Add object handling for typescript/es6-declarations #718
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for posting this PR! Would you be able to add unit tests to cover the new functionality as well? https://github.com/amzn/style-dictionary/blob/main/__tests__/common/formatHelpers/getTypeScriptType.test.js
Looks like we need to fix the broken unit tests as well: https://github.com/amzn/style-dictionary/runs/3969885050?check_suite_focus=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
This looks like a great addition; there are a few code cleanup requests I have made. In addition to those comments, also please migrate all functions to either declared functions or fat-arrow functions as the mix is a bit confusing. The easiest method is probably to make getObjectType() a declared function instead of fat-arrow.
Thanks again - looking forward to merging this in!
…/style-dictionary into fix/typescript-handling
Hey both! I've adjusted the PR. The code is cleaned-up and I've adjusted/enhanced the test to handle object situations. I've also adjusted the array case; arrays consisting out of multiple types now get returned like so:
compared to The output is bit cleaner now too (trailing commas have been removed, for example). Let me know what you think! [edit] Empty objects will now get returned as a Record<string, unknown> type instead of { } |
expect(getTypeScriptType(['an', 'array', 'of', 'strings'])).toEqual('string[]'); | ||
expect(getTypeScriptType([3.14159])).toEqual('number[]'); | ||
expect(getTypeScriptType([true, false, true, true])).toEqual('boolean[]'); | ||
}); | ||
|
||
it('should default to any, if no type is determined', () => { | ||
expect(getTypeScriptType({})).toEqual('any'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbanksdesign I removed these cases as we now actually handle objects
[edit] Created two new tests for validating any (based on passing null and undefined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hi there!
Our team is using this plugin to abstract tokens from a Figma file:
https://www.figma.com/community/plugin/888356646278934516/Design-Tokens
As you can see below, our typography tokens contain object with different properties that together form the typography style.
note I'm not entirely sure whenever this could/should be resolved in Figma or within the plugin itself, but I don't have much control over the source :-)
The issue we are facing with the default typescript/es6-declarations formatter, is that it doesn't handle objects, which results in the following types for the typography objects:
This PR changes this behaviour and recursively adds a complete typing for each token. It does so for each object separately (it doesn't create a type or interface and re-uses it), but at least it helps with handling these tokens in our UI library.