-
Notifications
You must be signed in to change notification settings - Fork 579
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
Changes from 1 commit
21a9765
366af52
2fb9854
2f9ebc0
90bf23a
1c28ba4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
} | ||
|
||
export default Action; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
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>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( I assume declaring the first group (ie. https://amzn.github.io/style-dictionary/#/config?id=configjson There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
export default Config; |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick, you may want to add a comment before |
||
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; |
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; |
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; |
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.
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.
(comments taken from https://amzn.github.io/style-dictionary/#/api?id=registeraction)
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.
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:
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
andNamed
type helper so the Action interface can be shared for both. Does that make sense? There might be a better way to accomplish that.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.
Sure, that makes sense.