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

fix(react-jss/types): fix react default props types support #1353

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

vismu
Copy link
Contributor

@vismu vismu commented Jun 10, 2020

What would you like to add/fix?

Hello, dear colleagues!
I fixed an issue connected with react default props in typescript.
You can read more about processing of react default props in typescript here:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html#support-for-defaultprops-in-jsx
In short, they calculate resulted props with help of JSX. LibraryManagedAttributes small function.

So, as you can see, in PR I used this helper to calculate component props types according official documentation.

Thanks for your attention!

Todo

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

@vismu vismu requested a review from HenriBeck as a code owner June 10, 2020 14:33
@vismu vismu force-pushed the fix/react-default-props-types-support branch from 70bb973 to 6b3c447 Compare June 10, 2020 14:34
@vismu vismu changed the title fix(react-jss/types): fix react default props types supoort fix(react-jss/types): fix react default props types support Jun 10, 2020
@kof
Copy link
Member

kof commented Jun 10, 2020

cc @moshest I have no idea, but if no one says this is bad or something I am going to merge it

@vismu please add a changelog after --- as improvement

Also can it be reflected in the test?

@vismu vismu force-pushed the fix/react-default-props-types-support branch from 6b3c447 to 0b0ab63 Compare June 10, 2020 15:07
@vismu
Copy link
Contributor Author

vismu commented Jun 10, 2020

@vismu please add a changelog after --- as improvement

Also can it be reflected in the test?

@kof Oh, it's just a typescript fix. So, I don't see a way how it could be tested.

I've updated changelog.md.

@moshest
Copy link
Member

moshest commented Jun 10, 2020

@vismu You can test it by adding a Typescript file under the test folder and use the types you defined. If there is any error there, tsc will catch that and block the build.

See here for example: https://github.com/cssinjs/jss/blob/master/packages/jss/tests/types/Styles.ts

@vismu
Copy link
Contributor Author

vismu commented Jun 10, 2020

@vismu You can test it by adding a Typescript file under the test folder and use the types you defined. If there is any error there, tsc will catch that and block the build.

See here for example: https://github.com/cssinjs/jss/blob/master/packages/jss/tests/types/Styles.ts

Nice, thank you for your advice! I'll do it.

@kof kof requested a review from moshest June 10, 2020 16:47
@vismu vismu force-pushed the fix/react-default-props-types-support branch from 0b0ab63 to d5a738b Compare June 10, 2020 19:55
Copy link
Member

@moshest moshest left a comment

Choose a reason for hiding this comment

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

Looking good to me.

@kof
Copy link
Member

kof commented Jun 10, 2020

Cool, one conflict to go and I am gonna merge it. Also Please check out other TS issues, mb there are simple fixes and I could get more stuff into that release

@vismu vismu force-pushed the fix/react-default-props-types-support branch from d5a738b to 0c8d61a Compare June 11, 2020 09:28
@vismu
Copy link
Contributor Author

vismu commented Jun 11, 2020

Cool, one conflict to go and I am gonna merge it. Also Please check out other TS issues, mb there are simple fixes and I could get more stuff into that release

Conflict was resolved.

Also I found a little issue that my test wasn't catch by tsc because of tsx extension. So I've added tsx files including in tsconfig.json.

About other TS issues. I found only one more and my PR is here: #1349

@@ -0,0 +1,24 @@
import React, {Component} from 'react'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should keep all ts tests in one file, otherwise there will be a mixture of js and tsx tests in that folder and that's weird because tsx tests are not jest tests like in those js files.

I think ideally we should move everything typescript into some dir in each package and have there both interfaces and tests

Copy link
Member

Choose a reason for hiding this comment

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

I realize this is not really part of the scope here, but with adding this file name it contributes to confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should keep all ts tests in one file, otherwise there will be a mixture of js and tsx tests in that folder and that's weird because tsx tests are not jest tests like in those js files.

I think ideally we should move everything typescript into some dir in each package and have there both interfaces and tests

Oh, thats right. I can do a subfolder types just by example of https://github.com/cssinjs/jss/blob/master/packages/jss/tests/types/Styles.ts
Also If you want I can change file name by your proposal.

Copy link
Member

@kof kof Jun 11, 2020

Choose a reason for hiding this comment

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

what about moving everything typescript into {packageName}/ts/... and inside for example
packages/jss/ts/index.d.ts, packages/jss/ts/index.test.ts or if tests are specific to some interface packages/jss/ts/styles.test.ts etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think that such folder reorganising doesn't make sense.
For me as for new contributor it is much clearer if all tests will be placed at tests folder including ts tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also have you noticed that tests dir is only used for some type of tests, all unit tests are directly located next to the code e.g. JssProvider.js, JssProvider.test.js

Oh, I didn't noticed that. But, follow that logic, I should put my test near the withStyles.js file and call it withStyles.test.tsx.

Copy link
Member

Choose a reason for hiding this comment

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

that is exactly the logic I am following, but the thing is this "tests" is testing the interface, so I consider the interfaces file to be "code" and that code using that interface - test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, think I understood your point. So I will put my test near the index.d.ts and call it index.test.tsx. Are you ok with that?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, for now that would be ok unless you want to move all ts things into {packageName}/ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I think it's not good idea move index.d.ts to another folder, because it should be placed right near index.js file. Thats how typescript will associate interfaces with implementation.

@vismu vismu force-pushed the fix/react-default-props-types-support branch from 64136e1 to 9418b45 Compare June 11, 2020 12:57
@kof kof merged commit 1c4feb7 into cssinjs:master Jun 12, 2020
@kof
Copy link
Member

kof commented Jun 12, 2020

merged!

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.

None yet

4 participants