fix(types): introduce Parser, update Config, optional Transform options #546

yoniholmes · 2021-02-19T13:47:10Z

Issue #, if available:
#545

Description of changes:

Add parsers & transform to the Config interface.
Add types for registerParser
Make options optional in the 3 Transform types ( NameTransform | ValueTransform | AttributeTransform)

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

yoniholmes · 2021-02-19T13:55:46Z

types/index.d.ts

@@ -45,6 +45,8 @@ declare namespace StyleDictionary {
  }

  interface Config {
+    parsers?: Parser[];
+    transform?: Record<string, Partial<Named<Transform>>>;


I'm not 100% sure what the expected usage of this transform record is. Based on the component-cti example, I see only transformer property has been defined: https://github.com/amzn/style-dictionary/blob/main/examples/advanced/component-cti/config.js#L26 (as opposed to including all possible fields: name, matcher, type, transitive)

So.... this is a substitute for calling .registerTransform(). It needs to match the internal structure for a transform which I believe is Transform in the current index.d.ts file.

https://github.com/amzn/style-dictionary/blob/main/examples/advanced/component-cti/config.js#L40-L46

Right... so I'm still a bit unclear about this I'm afraid!

registerTransform takes Named<Transform>> as it's input: https://github.com/amzn/style-dictionary/blob/3.0/types/index.d.ts#L170 (i.e. the Transform, plus a field called name).

However, transform in Config appears to take an object that maps the names of the transforms to the transform definitions. E.g.:

transform: { 'attribute/cti': CTITransform },

I typed this as Record<string, Partial<Named<Transform>>>. It's kind of equivalent to this which already exists:

interface Transforms { [name: string]: Transform; }

So I've changed this to use Transforms, which I think is better. The only gotcha I think will be that the CTI example will have a type error, because only the transformer field is supplied, it should really be supplying matcher and type as well. If however, it's possible to provide only some of those fields, then we should probably reflect this with Partial as in the original example (with or without Named as required). Sorry that's probably all rather confusing!

Take a look at the new version and let me know what you think.

If this is not helpful, I apologize in advance, as I'm doing like 7 things at once and have about 15 minutes left here before I have to be on childcare duty. I didn't want to leave you hanging until tomorrow though.... so hopefully this helps.

You have already seen registerTransform and registerTransformGroup, but for some additional examples, there are some great ones here (specifically in build.js): https://github.com/amzn/style-dictionary/tree/main/examples/advanced/custom-transforms

Now what does this have to do with the transform property in the config? Great question. When registerTransform is called, it simply does this: https://github.com/amzn/style-dictionary/blob/main/lib/register/transform.js#L59-L63

Wait, but that still isn't config - except, config works very similarly:

https://github.com/amzn/style-dictionary/blob/main/bin/style-dictionary#L84

https://github.com/amzn/style-dictionary/blob/main/lib/extend.js#L59-L138

That last link is a bit hard to interpret, but it is just modifying the StyleDictionary object and deep extends it by adding the configuration key/value pairs as properties (without overwriting).

Thus the type for transform should be the same in both places... I think. My amateur typescript skills showing again.

I hope this helps. If not, let me know and I can try to take a deeper look later.

Hopping in here. Taking a look at the 'component-cti' example, @yoniholmes is correct and the example is wrong (sorry!). The example should have at least a type attribute, matcher on a Transform is optional. If matcher is omitted, it will match all tokens. It looks like the what you have currently looks good to me. The transform attribute in the config takes named Transforms, and a Transform is the same whether used on there or in .registerTransform the only difference is where the name of the Transform goes. In .registerTransform it is added in a name attribute, when applied directly to the config the name is the key in the `transform object. That is a long-winded way of saying I think this looks good and the 'component-cti' example is wrong and should be failing.

Thanks @dbanksdesign I wasn't sure about the example. Appreciate the clarity.

yoniholmes · 2021-02-19T13:57:09Z

types/index.d.ts

+
+  interface Parser {
+    pattern: RegExp;
+    parse: (props: ParserOptions) => string | undefined;


I'm not sure whether a Parser always must return a string in all circumstances, hence string | undefined.

Parser should always return a plain JS object

I'm a little amateur at TypeScript, but I believe it should be...

parse: (props: ParserOptions) => Properties;

Thanks @chazzmoney, I've made this change. That makes a lot more sense!

chazzmoney

Thanks for this contribution and working to clean up the typescript.

I'm a bit amateur at TS, but I've tried to provide the best info I can. Let me know if anything I said doesn't make sense.

chazzmoney · 2021-02-23T00:25:01Z

types/index.d.ts

+
+  interface Parser {
+    pattern: RegExp;
+    parse: (props: ParserOptions) => string | undefined;


I'm a little amateur at TypeScript, but I believe it should be...

parse: (props: ParserOptions) => Properties;

chazzmoney · 2021-02-23T00:30:53Z

types/index.d.ts

@@ -45,6 +45,8 @@ declare namespace StyleDictionary {
  }

  interface Config {
+    parsers?: Parser[];
+    transform?: Record<string, Partial<Named<Transform>>>;


So.... this is a substitute for calling .registerTransform(). It needs to match the internal structure for a transform which I believe is Transform in the current index.d.ts file.

yoniholmes · 2021-02-23T15:28:06Z

types/index.d.ts

@@ -173,6 +186,7 @@ declare namespace StyleDictionary {
    registerTemplate(this: Core, template: Named<{ template: string }>): this;
    registerAction(this: Core, action: Named<Action>): this;
    registerFilter(this: Core, filter: Filter): this;
+    registerParser(this: Core, parser: Parser): this;


Incidentally... On further testing, I'm not sure exactly how registerParser is supposed to work. The types look ok. But when I initialised a style dictionary as follows, the parsers aren't used:

const myDictionary = require('style-dictionary').extend(myConfigObject) myDictionary.registerParser(myParser) myDictionary.buildAllPlatforms()

Also this didn't work:

const myDictionary = require('style-dictionary') myDictionary.registerParser(myParser) myDictionary.extend(myConfigObject) myDictionary.buildAllPlatforms()

The customer parsers are not executed. The only way I could get this to work was to put my custom parser inside myConfigObject, via: parsers: [myParser].

It looks like the extend function calls combineJSON before we've run registerParser, rather than waiting for buildAllPlatforms to run, hence registerParser is called too late. I don't suppose you have any examples how I'm suppose to run this?

Can you confirm you are on 3.0.0-rc4 version? I know there was an issue with parsers not being run on a previous release candidate version...

I just tried it out and these both work:

// build.js const StyleDictionary = require('style-dictionary'); const yaml = require('yaml'); const config = { // .. config }; StyleDictionary.registerParser({ pattern: /\.yaml$/, parse: ({contents, filePath}) => yaml.parse(contents) }); StyleDictionary.extend(config).buildAllPlatforms();

Where the script to build would be node build.js

// sd.config.js const StyleDictionary = require('style-dictionary'); const yaml = require('yaml'); const config = { // .. config }; StyleDictionary.registerParser({ pattern: /\.yaml$/, parse: ({contents, filePath}) => yaml.parse(contents) }); module.exports = config;

Where the script to run would be style-dictionary build --config ./sd.config.js

You are correct that you would need to register the parsers before you called .extend(). This is specific for parsers, where registering a transform for example could happen after .extend() but would have to happen before .buildAllPlatforms()

dbanksdesign

Great work @yoniholmes !

yoniholmes commented Feb 19, 2021

View reviewed changes

chazzmoney reviewed Feb 23, 2021

View reviewed changes

yoniholmes force-pushed the 3.0 branch from 6ca86f4 to 6615a91 Compare February 23, 2021 14:32

fix(types): introduce Parser, update Config, optional Transform options

93aae3e

yoniholmes force-pushed the 3.0 branch from 6615a91 to 93aae3e Compare February 23, 2021 14:40

yoniholmes commented Feb 23, 2021

View reviewed changes

dbanksdesign approved these changes Feb 27, 2021

View reviewed changes

dbanksdesign merged commit 0042354 into amzn:3.0 Feb 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types): introduce Parser, update Config, optional Transform options #546

fix(types): introduce Parser, update Config, optional Transform options #546

yoniholmes commented Feb 19, 2021

yoniholmes Feb 19, 2021 •

edited

Loading

chazzmoney Feb 23, 2021

chazzmoney Feb 23, 2021

yoniholmes Feb 23, 2021

chazzmoney Feb 25, 2021 •

edited

Loading

dbanksdesign Feb 27, 2021

chazzmoney Feb 27, 2021

yoniholmes Feb 19, 2021

chazzmoney Feb 23, 2021

chazzmoney Feb 23, 2021

yoniholmes Feb 23, 2021

chazzmoney left a comment

chazzmoney Feb 23, 2021

chazzmoney Feb 23, 2021

yoniholmes Feb 23, 2021 •

edited

Loading

dbanksdesign Feb 27, 2021

dbanksdesign left a comment

fix(types): introduce Parser, update Config, optional Transform options #546

fix(types): introduce Parser, update Config, optional Transform options #546

Conversation

yoniholmes commented Feb 19, 2021

yoniholmes Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chazzmoney Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chazzmoney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoniholmes Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbanksdesign left a comment

Choose a reason for hiding this comment

yoniholmes Feb 19, 2021 •

edited

Loading

chazzmoney Feb 25, 2021 •

edited

Loading

yoniholmes Feb 23, 2021 •

edited

Loading