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(types): cleaning up our type definitions #632

Merged
merged 6 commits into from
May 22, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion __tests__/formats/__snapshots__/all.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -744,7 +744,23 @@ exports[`formats all should match typescript/module-declarations snapshot 1`] =
*/

export default tokens;
declare interface DesignToken { value: string; name?: string; path?: string[]; comment?: string; attributes?: any; original?: any; }

declare interface DesignToken {
value: any;
name?: string;
comment?: string;
themeable?: boolean;
attributes?: {
category?: string;
type?: string;
item?: string;
subitem?: string;
state?: string;
[key: string]: any;
};
[key: string]: any;
}

declare const tokens: {
\\"color\\": {
\\"red\\": DesignToken
Expand Down
52 changes: 16 additions & 36 deletions __tests__/formats/typeScriptEs6Declarations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/
var fs = require('fs-extra');
var helpers = require('../__helpers');
var formats = require('../../lib/common/formats');
const formats = require('../../lib/common/formats');
const createDictionary = require('../../lib/utils/createDictionary');
const createFormatArgs = require('../../lib/utils/createFormatArgs');

var file = {
const file = {
"destination": "__output/",
"format": "typescript/es6-declarations",
"filter": {
Expand All @@ -24,46 +24,26 @@ var file = {
}
};

var dictionary = {
"allProperties": [{
"name": "red",
"value": "#EF5350",
"original": {
"value": "#EF5350"
},
"attributes": {
"category": "color",
"type": "base",
"item": "red",
"subitem": "400"
},
"path": [
"color",
"base",
"red",
"400"
]
}]
const properties = {
"color": {
"red": {"value": "#FF0000"}
}
};

var formatter = formats['typescript/es6-declarations'].bind(file);
const formatter = formats['typescript/es6-declarations'].bind(file);

describe('formats', () => {
describe('typescript/es6-declarations', () => {
beforeEach(() => {
helpers.clearOutput();
});

afterEach(() => {
helpers.clearOutput();
});

it('should be a valid TS file', () => {
const declarations = './__tests__/__output/output.d.ts';
fs.writeFileSync(declarations, formatter(dictionary) );
const dictionary = createDictionary({ properties });
const output = formatter(createFormatArgs({
dictionary,
file,
platform: {},
}), {}, file);

// get all lines that begin with export
const lines = fs.readFileSync(declarations, 'utf-8')
const lines = output
.split('\n')
.filter(l => l.indexOf('export') >= 0);

Expand Down
35 changes: 14 additions & 21 deletions __tests__/formats/typeScriptModuleDeclarations.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/
var fs = require('fs-extra');
var helpers = require('../__helpers');
var formats = require('../../lib/common/formats');
const formats = require('../../lib/common/formats');
const createDictionary = require('../../lib/utils/createDictionary');
const createFormatArgs = require('../../lib/utils/createFormatArgs');

var file = {
const file = {
"destination": "__output/",
"format": "typescript/module-declarations",
"filter": {
Expand All @@ -24,11 +24,9 @@ var file = {
}
};

var dictionary = {
"properties": {
"color": {
"red": {"value": "#FF0000"}
}
const properties = {
"color": {
"red": {"value": "#FF0000"}
}
};

Expand All @@ -37,20 +35,16 @@ var formatter = formats['typescript/module-declarations'].bind(file);

describe('formats', () => {
describe('typescript/module-declarations', () => {
beforeEach(() => {
helpers.clearOutput();
});

afterEach(() => {
helpers.clearOutput();
});

it('should be a valid TS file', () => {
const declarations = './__tests__/__output/output-module.d.ts';
fs.writeFileSync(declarations, formatter(dictionary) );
const dictionary = createDictionary({ properties });
const output = formatter(createFormatArgs({
dictionary,
file,
platform: {},
}), {}, file);

// get all lines that have DesignToken
const lines = fs.readFileSync(declarations, 'utf-8')
const lines = output
.split('\n')
.filter(l => l.indexOf(': DesignToken') >= 0);

Expand All @@ -60,5 +54,4 @@ describe('formats', () => {
});
});
});

});
38 changes: 27 additions & 11 deletions lib/common/formats.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/

const fs = require('fs');
const path = require('path');
const _template = require('lodash/template');
const GroupMessages = require('../utils/groupMessages');
const { fileHeader, formattedVariables, iconsWithPrefix, minifyDictionary, sortByReference, createPropertyFormatter, sortByName } = require('./formatHelpers');
Expand Down Expand Up @@ -389,8 +390,8 @@ module.exports = {
* export const ColorBackgroundAlt : string;
* ```
*/
'typescript/es6-declarations': function(dictionary) {
return fileHeader(this.options) +
'typescript/es6-declarations': function({dictionary, file}) {
return fileHeader({file}) +
dictionary.allProperties.map(function(prop) {
var to_ret_prop = 'export const ' + prop.name + ' : string;';
if (prop.comment)
Expand Down Expand Up @@ -452,7 +453,8 @@ module.exports = {
* });
* ```
*/
'typescript/module-declarations': function(dictionary) {
'typescript/module-declarations': function({dictionary, file, options}) {
const {moduleName=`tokens`} = options;
function treeWalker(obj) {
let type = Object.create(null);
let has = Object.prototype.hasOwnProperty.bind(obj);
Expand All @@ -468,13 +470,27 @@ module.exports = {
}
return type;
}
const designTokenInterface = fs.readFileSync(
path.resolve(__dirname, `../../types/DesignToken.d.ts`), {encoding:'UTF-8'}
);

// get the first and last lines to add to the format by
// looking for the first and last single-line comment
const lines = designTokenInterface
.split('\n');
const firstLine = lines.indexOf(`//start`) + 1;
const lastLine = lines.indexOf(`//end`);

const output = fileHeader({file}) +
`export default ${moduleName};

var file = fileHeader(this.options) +
'export default tokens;\n' +
'declare interface DesignToken { value: string; name?: string; path?: string[]; comment?: string; attributes?: any; original?: any; }\n' +
'declare const tokens: ' +
JSON.stringify(treeWalker(dictionary.properties), null, 2);
return file.replace(/"DesignToken"/g,'DesignToken');
${lines.slice(firstLine, lastLine).join(`\n`)}

declare const ${moduleName}: ${JSON.stringify(treeWalker(dictionary.tokens), null, 2)}`;

// JSON stringify will quote strings, because this is a type we need
// it unquoted.
return output.replace(/"DesignToken"/g,'DesignToken');
},

// Android templates
Expand Down Expand Up @@ -683,9 +699,9 @@ module.exports = {
* @example
* ```kotlin
* package com.example.tokens;
*
*
* import androidx.compose.ui.graphics.Color
*
*
* object StyleDictionary {
* val colorBaseRed5 = Color(0xFFFAF3F2)
* }
Expand Down
22 changes: 22 additions & 0 deletions types/Action.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/

import Dictionary from './Dictionary';
import Platform from './Platform';

interface Action {
do(dictionary: Dictionary, config: Platform): void;
undo?(dictionary: Dictionary, config: Platform): void;
}
Copy link
Contributor

@luke-john luke-john May 14, 2021

Choose a reason for hiding this comment

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

I see this is just isolating an existing type, so enhancing them is probably best left for a future pr.

However looking at https://amzn.github.io/style-dictionary/#/api?id=registeraction it seems this definition is incomplete (missing name). (this lack of completeness applies to many types still, I've not commented elsewhere)

An additional enhancement for consumers, would be to add jsdoc comments to help communicate usage to users. (again probably best left for a future pr)

ie.

interface Action {
  /** The action in the form of a function. */
  do(dictionary: Dictionary, config: Platform): void;
  /** A function that undoes the action. */
  undo?(dictionary: Dictionary, config: Platform): void;
}

(comments taken from https://amzn.github.io/style-dictionary/#/api?id=registeraction)

Copy link
Member Author

Choose a reason for hiding this comment

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

For all of the register-able things (action, format, filter, fileHeader, transform, transformGroup, parser) I isolated the thing itself (an action for example) from how you would register it. In Style Dictionary you can add custom things 2ways: with a register method, or by sticking it directly in a configuration object:

StyleDictionary.extend({
  action: {
    actionName: { do: () => {}, undo: () => {} }
  }
});

In this way, the "name" of the action is outside the action object, and is the key to that object. To handle both scenarios I created a Keyed and Named type helper so the Action interface can be shared for both. Does that make sense? There might be a better way to accomplish that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that makes sense.


export default Action;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not encountered this pattern in typescript before where a type is exported via a default, and was surprised it worked.

While it does work, it's very unusual in typescript (I've never encountered this before).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my "non-typescript person trying to write typescript" coming out. What is the conventional way to do this then? I did have a lot of weirdness trying to split up the type definition files and import/export them. Would love to know the better way to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

While exploring the jsdoc setup, I've created a branch with the types adjusted to use named exports (and the definition refactored to a module) that you can check out.

https://github.com/amzn/style-dictionary/compare/fixing-typescript...luke-john:fixing-typescript-explore?expand=1

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that is awesome! Thank you so much for taking the time and doing that. I really appreciate it.

thank you so much

41 changes: 41 additions & 0 deletions types/Config.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/

import { Keyed } from './_helpers';

import Parser from './Parser';
import Transform from './Transform';
import TransformGroup from './TransformGroup';
import Filter from './Filter';
import FileHeader from './FileHeader';
import Format from './Format';
import Action from './Action';
import Platform from './Platform';
import { DesignTokens } from './DesignToken';

interface Config {
parsers?: Parser[];
transform?: Keyed<Transform>;
transformGroup?: Keyed<TransformGroup>;
format?: Keyed<Format>;
filter?: Keyed<Filter>;
fileHeader?: Keyed<FileHeader>;
action?: Keyed<Action>;
include?: string[];
source?: string[];
tokens?: DesignTokens;
properties?: DesignTokens;
platforms: Keyed<Platform>;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unclear to me if this is correct, on the website a number of these properties seem to be nested under Platform, and don't appear on Config (transform , transformGroup, fileHeader) and some are not documented at all (parsers, tokens).

I assume declaring the first group (ie. transform) here by name, allows you to use it inside platforms by name. Nitpick: Seeing as this is the first time it's being documented, it may be worth adding JSDoc comments to the types here explaining usage.

https://amzn.github.io/style-dictionary/#/config?id=configjson

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not documented on the main branch, but have been updated in 3.0. If you look at my comment above, the .extend method can accept a transform object to define custom transforms inline in the configuration directly rather than calling .registerTransform. Some examples:

}

export default Config;
36 changes: 36 additions & 0 deletions types/DesignToken.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/

//start
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, you may want to add a comment before //start to indicate that it is being used by an internal tool (formats.js), so that additional changes in future keep that in mind.

declare interface DesignToken {
value: any;
name?: string;
comment?: string;
themeable?: boolean;
attributes?: {
category?: string;
type?: string;
item?: string;
subitem?: string;
state?: string;
[key: string]: any;
};
[key: string]: any;
}
//end

export interface DesignTokens {
[key: string]: DesignTokens | DesignToken;
}

export default DesignToken;
25 changes: 25 additions & 0 deletions types/Dictionary.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/

import TransformedToken, {TransformedTokens} from './TransformedToken';

interface Dictionary {
allTokens: TransformedToken[];
tokens: TransformedTokens;
allProperties: TransformedToken[];
properties: TransformedTokens;
usesReference: (value: any) => boolean;
getReferences: (value: any) => TransformedToken[];
}

export default Dictionary;
24 changes: 24 additions & 0 deletions types/File.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance with
* the License. A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
* CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/

import Options from './Options';
import TransformedToken from './TransformedToken';

interface File {
destination: string;
format?: string;
filter?: string | Partial<TransformedToken> | ((token: TransformedToken) => boolean);
options?: Options;
}

export default File;
Loading