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

Restore TypeScript support for Observable styles #1402

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

appsforartists
Copy link
Member

@appsforartists appsforartists commented Oct 5, 2020

Corresponding issue (if exists):

What would you like to add/fix?

const jss = createJSS(createDefaultJSSPreset());
jss.createStyleSheet(
  {
    myClassName: someObservable$,
  },
  {
    link: true,
  },
).attach();

should work in TypeScript, and used to work with @types/jss. JSS now supplies its own types, which don't include Observable support. This PR adds them back.

As @worudso alluded to in #1035, a better solution would only include the Observable type if users had installed the Observable plugin. Otherwise, users could be frustrated if the typecheck passes for createStyleSheet(someObservables) but it fails at runtime.

However, that solution would require bigger and more thoughtful changes to the typings. This PR is a quick fix that addresses the regression from @types/jss, while leaving the work of a refactor to a future PR.

Todo

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

@appsforartists appsforartists force-pushed the observable-type branch 3 times, most recently from 04dcddd to ca24b90 Compare October 5, 2020 20:43
These look like they were ported over from @types/jss
(https://github.com/DefinitelyTyped/DefinitelyTyped/blob/de655960b603d6b47f7030674f084780c76e045f/types/jss/jss-tests.ts).
However, if you make them incorrect (e.g. `$ExpectType boolean` for everything), `yarn run check:ts` doesn't error.

Unused tests are worse than no tests, so I'm removing them.
@appsforartists appsforartists changed the title Add Observable support to types Restore TypeScript support for Observable styles Oct 6, 2020
@kof
Copy link
Member

kof commented Oct 7, 2020

lgtm, thanks!

@kof kof merged commit 56f54f5 into cssinjs:master Oct 7, 2020
@kof
Copy link
Member

kof commented Nov 15, 2020

released https://github.com/cssinjs/jss/releases/tag/v10.5.0

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.

2 participants