search

cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.06k stars 397 forks source link

[all] Add tests for Typescript definition files #964

Open rosskevin opened 5 years ago

rosskevin commented 5 years ago

Expected behavior: Any typescript definitions exported from this library should be tested, otherwise it can cause real pain and doubt for ts users.

Describe the bug: https://github.com/cssinjs/jss/pull/889#issuecomment-447220080 was integrated without tests. It did not start with @types/jss package and we have no guarantee compatibility.

Versions (please complete the following information):

Context Changelog from 9->10 should address user's removal of @types/jss as a requirement otherwise conflicts will arise. As-is, I am seeing app errors downstream of material-ui 3.8.1 because of a mix of type use between 9.x and 10.x. It is not easy or apparent there, but is within our app. Investigating in material-ui, different packages require different versions of jss, but ultimately the root of the yarn workspace has 10.alpha.3 and @types/jss. It just so happens that the tests/usage there passes, but does not in the downstream app.

For samples of typescript testing, check out material-ui https://github.com/mui-org/material-ui and usage tests such as https://github.com/mui-org/material-ui/tree/master/packages/material-ui/test/typescript

/cc @appsforartists @HenriBeck

HenriBeck commented 5 years ago

Yes. Adding compatibility tests are planned.

Quick question: why has an official version of material-ui an alpha from jss?

rosskevin commented 5 years ago

I'm not sure, I don't have an answer yet - I think olivier is probably celebrating new years. Probably unintended but I did find it in the monorepo root "resolutions". I think it was intended to be performance testing for another alpha mui package and...unintentionally...since it was in the root resolutions runtime types (which were scoped to 9.x and @types/jss) ended up getting updated to 10.alpha instead of staying on @types/jss.

Actual details here: https://github.com/mui-org/material-ui/issues/14040#issuecomment-450684391

Intentions yet to be determined.

rosskevin commented 5 years ago

@HenriBeck it looks like alpha dependency was unintentional for @material-ui/core, and intended only for new public package (also alpha) of @material-ui/styles. Bug/error in package.json(s).

appsforartists commented 5 years ago

Reiterating my comment in #889: Why is the index.d.ts in this repo not based on @types/jss? Is there a perceived deficiency in the @types version?

The @types version has some nice features, like checking that the key names in classes are correct, that don't seem to be in the official version, and as @rosskevin points out, most existing JSS TS users are depending on them. (They have .25MM weekly downloads on npm.)

If JSS wants to maintain the types, I strongly suggest we base them on all the great work that's gone into the existing package. Replacing the community version with breaking changes and fewer features smells NIH. I don't know if that's the case or not, but it's the perception.

HenriBeck commented 5 years ago

Breaking changes are anyways required as we changed the name of createGenerateClassname to createGenerateId.

I think we can update the current definitions to include such features. I was hesitant to just copy them over as the Styles type might be wrong/incomplete.