feat(types): cleaning up our type definitions #632
Conversation
There was a problem hiding this comment.
Really nice work on this 👍 , my comments are mostly stylistic notes.
disclaimer
I don't have access to a decent sized code base which uses style-dictionary, and so don't have a good way to test the type changes.
If you're able to pre publish a test version of these, @didoo may be able to run them against a codebase which would possibly pick up issues if they had been introduced.
suggestion for consideration, using these types in the js source files
Between these type files, the documentation website, and the jsdoc types in the source files, things are now being documented across three places 😅.
To help reduce this back towards two, you may want to consider replacing the jsdoc types in the source files with the types in these files. This would not just help keep the types up to date, but can also improve the development experience when working on style-dictionary.
ie. lib/cleanFiles.js (note: the params here assume the type files have had the default exports removed)
// this comment means you'll get type warnings in vscode (and if you wanted, could also run the typescript compiler on your source files to get some level of jsdoc type validation.
// @ts-check
/**
* Takes a platform config object and a dictionary
* object and cleans all the files. Dictionary object
* should have been transformed and resolved before this
* point.
* @memberOf StyleDictionary
* @param { import('../types/Dictionary').Dictionary } dictionary
* @param { import('../types/Platform').Platform } platform
* @returns { void }
*/
function cleanFiles(dictionary, platform) {unrelated observation
And finally a note on something you haven't changed, but I noticed while looking at the index.d.ts file.
export as namespace StyleDictionary;This is setting it up to behave as a UMD module (see https://www.typescriptlang.org/docs/handbook/modules.html#umd-modules). I'm not sure this is something that reflects the actual behaviour of styled-dictionary.
types/Action.d.ts
Outdated
| undo?(dictionary: Dictionary, config: Platform): void; | ||
| } | ||
|
|
||
| export default Action; |
There was a problem hiding this comment.
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.
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.
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.
Oh that is awesome! Thank you so much for taking the time and doing that. I really appreciate it.
types/Action.d.ts
Outdated
| interface Action { | ||
| do(dictionary: Dictionary, config: Platform): void; | ||
| undo?(dictionary: Dictionary, config: Platform): void; | ||
| } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
types/Config.d.ts
Outdated
| source?: string[]; | ||
| tokens?: DesignTokens; | ||
| properties?: DesignTokens; | ||
| platforms: Keyed<Platform>; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
| * and limitations under the License. | ||
| */ | ||
|
|
||
| //start |
There was a problem hiding this comment.
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.
types/_helpers.ts
Outdated
| */ | ||
|
|
||
| type Named<T> = T & { name: string; }; | ||
| type Keyed<T> = { [name: string]: T }; |
There was a problem hiding this comment.
Just a note, typescript comes with a utility type Record which allows something similar.
So consumers could instead write Record<string, Type>.
Though, from looking at consumers, it seems this type is being used to indicate such maps where the key becomes an alias to the value that is available for usage elsewhere. nitpick: In which case adding a comment here, or giving a more descriptive name may help.
| pattern: RegExp; | ||
| parse: (props: ParserOptions) => Properties; | ||
| } | ||
| declare namespace StyleDictionary { |
There was a problem hiding this comment.
Just a note, the TypeScript docs recommend "modules over namespaces in modern code."
https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html#using-modules
To transform this file, you would need to
delete the namespace wrapper
- declare namespace StyleDictionary {
- }update the types inside it to be exported
- type DesignToken = _DesignToken;
+ export type DesignToken = _DesignToken; Or, if you also removed the default exporting of types in many of the files, you could replace them so you just reexport the original types.
- import _File from "./File";
- type File = _File;
+ export { File } from "./File";and finally update the main export.
- declare var StyleDictionary: StyleDictionary.Core;
- export = StyleDictionary;
+ declare var StyleDictionary: Core;
+ export default StyleDictionary;There was a problem hiding this comment.
I knew doing this felt so wrong:
import _File from "./File";
type File = _File;There was a problem hiding this comment.
I think we might need to revert back to the namespace style or else it might mess up the autocomplete/intellisense for JS users.
|
Thank you for the comments @luke-john! 100% agree about having documentation spread in multiple places. I'll try your suggestion of importing the types in the JSDoc comments. Could we also put JSDoc comments in the type files and then reference those in the JS files? It is hard to strike a balance of adding types, but without re-writing the whole codebase 😓 |
|
@luke-john I get a JSDoc error when I try to run JSDoc with the |
|
I hadn't noticed that you were using jsdoc-api to generate some of the website documentation when I made that comment. I just had a look into it, and I can't find a simple way to get jsdoc to understand those types. There's a tracking issue on jsdoc for something similar, but it does not seem very active jsdoc/jsdoc#1645. There are a number of alternatives, which will understand both jsdoc + typescript (such as typedoc), but that would be a fairly significant rework in and off itself. And while I'd be happy to help with migrating to such an alternative (or just going all in on typescript 😅), it would definitely be in "re-writing the whole codebase" territory 😓 . Also you could import the js files jsdoc types into the types files. ie. // index.d.ts
type RegisterAction = typeof import("../lib/register/action");
export interface Core {
registerAction: RegisterAction
}
// RegisterAction.d.ts
export type RegisterAction = typeof import("../lib/register/action");
type RegisterActionOptions = Parameters<RegisterAction>[0]
export type Action = Omit<
RegisterActionOptions,
'name'
>This would result in the typescript type for registerAction to be type RegisterAction = (action: {
name: string;
do: Function;
undo: Function;
}) => anyBut you'll notice here that;
So I would not recommend it. |
types/index.test-d.ts
Outdated
| expectType<StyleDictionary.File>({ | ||
| format: `css/variables`, | ||
| destination: `variables.css` | ||
| } as StyleDictionary.File); |
There was a problem hiding this comment.
I missed this while first reviewing, the as keyword in typescript is a type assertion. So here you are saying to typescript to force the type of that object into StyleDictionary.File, which means even if the object did not match that type, this would pass.
While TypeScript only allows type assertions which convert to a more specific or less specific version of a type ("impossible conversions" such as 1 as StyleDictionary.File), it will not prevent transforms such as {format: 'css/variables'} as StyleDictionary.File (File is typed to always require a destination).
An alternative here would be to use the expectAssignable from tsd, which will ensure that this object can be assigned to StyleDictionary.File
|
@luke-john I made a bunch of the updates you suggested if you want to take a look. It was all super helpful. Here is what I am proposing: if these changes look good we can merge these in for now with the expectation that we will come up with a better long-term strategy for typescript + code documentation. I think these changes are better than what style dictionary currently has, so at least it should be an improvement. But now it looks like the build is failing. Also, when I try to use this new version locally, it messes up VSCode's JS intellisense. Switching the namespace stuff to module exports messes it up. Here is what it looks like:
If I run that file, I can actually access all the stuff inside Style Dictionary I would expect like |
|
Update: reverting back to the namespace way or else we lose autocomplete support in VSCode for non-typescript users. That is a bit of a non-starter. If there is a way to support CommonJS without using the namespace thing I'm all for it. |
|
Hey, sorry for sending you down a rabbit hole with the commonjs support. Looks like the namespace + While having a look into it, I noticed that the files field in package.json explicitly contains You'll want to either change that to something like |
|
Thanks for help! Sorry this has been a bit of a mess. |
dbanksdesign
force-pushed
the
fixing-typescript
branch
from
307bbf1 to
1c28ba4
Compare
Issue #, if available:
Description of changes:
DesignTokentype fortypescript/module-definitionsformat as well as in our typesI'm not a Typescript person so would love some expert 👀 on this.
update
Here is what I am thinking for this pull request: is this code good enough for now? Good enough meaning: is it a better developer experience for Javascript and Typescript developers than what currently exists in the live version of Style Dictionary (2.10)? If so, and there are no obvious errors with the types, then I think we can merge in and get 3.0 out, and then plan on cleaning up in a minor release afterwards.
Some things that are not great, but acceptable for now to not push the 3.0 launch out even further to migrate the whole codebase to Typescript.
Other avenues I tried:
tscwith the existing source JS files to auto-generate types. This worked somewhat, but it had some downsides. It did not work well trying to share types across different parts of the codebase, like a Transform or DesignToken for example. It also did not export all types in the main entry point to the library. To get this to work well, it would require a lot more tweaking and editing of the source code, which at that point we might as well migrate to typescript.In maybe a new minor version (3.1) we could migrate the codebase to Typescript and clean this up a bit.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.