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 fixes for #1308 and #1309 #1310

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

dfernandez-asapp
Copy link
Contributor

Corresponding issue (if exists):

#1308
#1309

What would you like to add/fix?

This PR changes the type definition of createUseStyles<Theme>(styles, options?: CreateUseStyles) so it passes the Theme type parameters to the options type: createUseStyles<Theme>(styles, options?: CreateUseStyles<Theme>).

It also re-exports the Theming type, so users of createTheming can do explicit typing of the result: const theming : Theming = createTheming(... without having to add the theming package to the direct dependencies.

Todo

  • Add tests if possible
  • Add changelog if users should know about the change
  • Add documentation

@kof
Copy link
Member

kof commented Mar 16, 2020

Is this considered a breaking change?

@dfernandez-asapp
Copy link
Contributor Author

Is not a breaking change:

  • If you don't use TypeScript you are not affected by this.
  • If you use TypeScript, expressions like createUseStyles<XYZ>(), createTheming(Context) or createTheming<XYZ>(Context) are not affected since the generic argument for the options will take a default (that is the same as before {}).

The main difference is that before this PR you can't pass a custom theming to createUseStyles in TypeScript without having to cast it to Theming<{}> and to cast it to Theming you need to import the type from the theming module.

A few things not included in the PR:

  • I didn't add any tests, because I didn't see any typings tests (eg dtslint or tsd) in the project. This is a compile time check that cannot be tested with a normal unit test.
  • I didn't do any change to Flow typings, I don't use Flow and I don't know if the Flow types definition has the same issue or not.

@kof
Copy link
Member

kof commented Mar 16, 2020

I am going to merge it and release with the next release, but since I already released yesterday, I am gonna wait for more PRs to get merged.

@kof kof merged commit 2a51d70 into cssinjs:master Mar 16, 2020
@kof
Copy link
Member

kof commented Mar 16, 2020

Merged, Thanks

@kof kof added the typescript label Mar 16, 2020
@kof
Copy link
Member

kof commented Jun 3, 2020

Released in v10.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants