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

Add object handling for typescript/es6-declarations #718

Merged
merged 8 commits into from
Oct 28, 2021

Conversation

RobbyUitbeijerse
Copy link
Contributor

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.

export const ColorInteractionNotifyLight = "#ffefe6";
export const ColorInteractionFocus = "#0353e9";
export const FontHeadingH1Title = {"fontSize":72,"textDecoration":"none","fontFamily":"Clash Display","fontWeight":600,"fontStyle":"normal","fontStretch":"normal","letterSpacing":0.72,"lineHeight":72,"paragraphIndent":0,"paragraphSpacing":0,"textCase":"none"};
export const FontHeadingH2Title = {"fontSize":60,"textDecoration":"none","fontFamily":"Clash Display","fontWeight":600,"fontStyle":"normal","fontStretch":"normal","letterSpacing":0.6,"lineHeight":62,"paragraphIndent":0,"paragraphSpacing":0,"textCase":"none"};

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:

export const ColorInteractionNotifyLight : string;
export const ColorInteractionFocus : string;
export const FontHeadingH1Title : any;
export const FontHeadingH2Title : any;

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.

Copy link
Member

@dbanksdesign dbanksdesign left a 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

lib/common/formatHelpers/getTypeScriptType.js Outdated Show resolved Hide resolved
@dbanksdesign
Copy link
Member

Looks like we need to fix the broken unit tests as well: https://github.com/amzn/style-dictionary/runs/3969885050?check_suite_focus=true

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.

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!

lib/common/formatHelpers/getTypeScriptType.js Outdated Show resolved Hide resolved
lib/common/formatHelpers/getTypeScriptType.js Outdated Show resolved Hide resolved
lib/common/formatHelpers/getTypeScriptType.js Outdated Show resolved Hide resolved
@RobbyUitbeijerse
Copy link
Contributor Author

RobbyUitbeijerse commented Oct 22, 2021

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:

(string | number)[]

compared to any[] earlier.

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');
Copy link
Contributor Author

@RobbyUitbeijerse RobbyUitbeijerse Oct 22, 2021

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)

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!

:shipit:

Copy link
Member

@dbanksdesign dbanksdesign left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@dbanksdesign dbanksdesign merged commit 4e3905a into amzn:main Oct 28, 2021
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.

3 participants