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

[all] Add tests for Typescript definition files #964

Open
rosskevin opened this issue Dec 31, 2018 · 5 comments
Open

[all] Add tests for Typescript definition files #964

rosskevin opened this issue Dec 31, 2018 · 5 comments
Assignees
Labels
complexity:moderate We talked about it, you can do it! help wanted important The thing you do when you wake up! typescript

Comments

@rosskevin
Copy link

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:
#889 (comment) was integrated without tests. It did not start with @types/jss package and we have no guarantee compatibility.

Versions (please complete the following information):

  • jss: 9 to 10.alpha.3

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

@rosskevin rosskevin changed the title typescript is not tested alpha/in-repo typescript is not tested Dec 31, 2018
@HenriBeck
Copy link
Member

Yes. Adding compatibility tests are planned.

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

@rosskevin
Copy link
Author

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: mui/material-ui#14040 (comment)

Intentions yet to be determined.

@rosskevin
Copy link
Author

@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).

@kof kof added important The thing you do when you wake up! complexity:moderate We talked about it, you can do it! labels Dec 31, 2018
@appsforartists
Copy link
Member

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
Copy link
Member

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.

@HenriBeck HenriBeck self-assigned this Jan 4, 2019
@cssinjs cssinjs deleted a comment from glenveegee Jan 24, 2019
@cssinjs cssinjs deleted a comment from HenriBeck Jan 24, 2019
@cssinjs cssinjs deleted a comment from glenveegee Jan 24, 2019
@cssinjs cssinjs deleted a comment from glenveegee Jan 24, 2019
@cssinjs cssinjs deleted a comment from glenveegee Jan 24, 2019
@HenriBeck HenriBeck changed the title alpha/in-repo typescript is not tested [all] Add tests for Typescript definition files Jan 26, 2019
@kof kof added the typescript label Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity:moderate We talked about it, you can do it! help wanted important The thing you do when you wake up! typescript
Projects
None yet
Development

No branches or pull requests

4 participants