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

typescript type definitions #339

Closed
wants to merge 4 commits into from
Closed

typescript type definitions #339

wants to merge 4 commits into from

Conversation

zxbodya
Copy link
Contributor

@zxbodya zxbodya commented Sep 23, 2019

While building a tool for flow to typescript migration, I used styletron as one of the projects to test it. And so - now I have typescript version of styletron(https://github.com/zxbodya/styletron/tree/ts)

Type definitions in this PR are generated from that typescript version.

While doing it, I was trying to make it compatible with definitions currently in DefinitelyTyped(ensuring things like type parameters order in generics to be the same), also I did some additional corrections while using it with migrated version of baseui

For the most part(except maybe some places where flow types were not very accurate) - new type definitions are more accurate comparing to the version in DefinitelyTyped.

btw, this should also help with #337

Copy link

@choyongjoon choyongjoon left a comment

Choose a reason for hiding this comment

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

There are many anys compared to DefinitelyTyped's one. How about to check all anys and get rid of them as many as possible?

children: React.ReactNode;
value: StandardEngine;
debugAfterHydration?: boolean;
debug?: any;

Choose a reason for hiding this comment

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

Suggested change
debug?: any;
debug?: BrowserDebugEngine | NoopDebugEngine;

}
export declare const Provider: typeof DevProvider | React.ProviderExoticComponent<React.ProviderProps<StandardEngine>>;
export declare function DevConsumer(props: {
children: (c: any, b: any, a: any) => React.ReactNode;

Choose a reason for hiding this comment

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

Suggested change
children: (c: any, b: any, a: any) => React.ReactNode;
children: (
styletronEngine: StandardEngine,
debugEngine: BrowserDebugEngine | NoopDebugEngine,
hydrating: boolean
) => React.ReactNode;

@zxbodya
Copy link
Contributor Author

zxbodya commented Sep 26, 2019

There are many anys compared to DefinitelyTyped's one. How about to check all anys and get rid of them as many as possible?

that would be great to do this

but, it is kind of outside of the scope of what I was doing - my goal was more about to convert as much of existing flow types as possible, and to reveal pain points in converted code by trying to do fixes needed after migration from flow(btw, I also have converted baseui codebase, to reveal what additional fixes in styletron needed to support usages in baseui). It was not really a goal to provide complete type coverage… and things which are any are actually not covered in flow types

however, if you feel like doing it - you can try doing it on top of ts branch (https://github.com/zxbodya/styletron/tree/ts) - I would be happy to update this pr to include those changes, also btw - it might be even better adding it as flow type annotations in sources (it is relatively easy to update ts version to include changes in flow sources)

other option about doing it(which I am planning to try), would be to automate generating d.ts files and rebasing fixes on top of generated files - then it would be easier to backport types from definitely typed(and also to require less effort to create typescript typings from existing sources with flow type annotations)

@tajo
Copy link
Member

tajo commented Jul 21, 2020

new PR based off this one: #371

@tajo tajo closed this Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants