amzn / style-dictionary

A build system for creating cross-platform styles.
https://styledictionary.com
Apache License 2.0
3.87k stars 543 forks source link

[Question] `options` argument type in Transform.transformer is not optional #975

Closed m-yoshiro closed 1 year ago

m-yoshiro commented 1 year ago

Hello everyone,

I'm having a problem with style-dictionary@3.8.0 and Typescript. When running tests in my project, this error occurred.

I think the problem came from https://github.com/amzn/style-dictionary/pull/926, where Transform.transformer type was modified. options argument has been added and its type is not optional.

I'd like to ask if this behavior is intentional or not. Thanks!

    src/transformers/typographyToCss.test.ts:59:42 - error TS2554: Expected 2 arguments, but got 1.

    59     const cssFontValue = typographyToCss.transformer(mockToken);
                                                ~~~~~~~~~~~~~~~~~~~~~~

      node_modules/style-dictionary/types/Transform.d.ts:23:5
        23     options: Platform<PlatformType>
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        An argument for 'options' was not provided.

p.s. I found that some transfomer's tests don't use options argument, so I suppose it might be unintentional.

https://github.com/amzn/style-dictionary/blob/2cf72f3f89996503608c238d7f3bcbab5b53e719/__tests__/common/transforms.test.js#L607-L610


In Addition, my typographyToCss 's implementation and test code are like below.

// Implementation
export const typographyToCss: Transform = {
  type: 'value',
  transitive: true,
  matcher: isTypography,
  transformer: (token) => {
    const { fontFamily, fontWeight, lineHeight, fontSize } = token.original
      .value as TypographyTokenValue;

    const output = `${fontWeight} ${fontSize}/${lineHeight} ${fontFamily}`;
    return output;
  },
};
// Test
const cssFontValue = typographyToCss.transformer(mockToken);

expect(cssFontValue).toBe(
  `${fontWeight} ${fontSize}/${lineHeight} ${fontFamily}`
);
dbanksdesign commented 1 year ago

Thank you for the question @m-yoshiro! That change was intentional because when Style Dictionary calls that function, it always passes in the platform configuration object, so we want the custom transformer functions to not have to do an undefined check in their transformer function. This does make calling the transformer function in a unit test a bit more difficult, but you can just add an empty object as the 2nd argument in your unit test.

I'm open to suggestions on how to improve the typing of this though.

m-yoshiro commented 1 year ago

Thank you for your explanations! I understand the reason and It's clear to me now.