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

Add typings for module and submodules #21

Closed
wants to merge 1 commit into from

Conversation

stramel
Copy link
Contributor

@stramel stramel commented Nov 15, 2019

Adds typings for the main module and submodules.

@stramel
Copy link
Contributor Author

stramel commented Nov 15, 2019

The DX of these hooks with typings feels pretty rough. Open to suggestions

@stramel stramel mentioned this pull request Nov 19, 2019
@addyosmani addyosmani self-requested a review November 20, 2019 16:09
@addyosmani
Copy link
Collaborator

Thank you for working on a PR adding typings, @stramel. I see there's already been a request for this in #27. I would like to keep this open to give (1) more users a chance to signal if typings are important to them and (2) more reviewers a chance to chime in on DX.

@addyosmani addyosmani added the chillin' ❄️ Chillin' (awaiting further feedback) label Nov 20, 2019
@stramel stramel mentioned this pull request Nov 20, 2019
@stramel
Copy link
Contributor Author

stramel commented Nov 26, 2019

I think to fix the DX of using types, we always return unsupported as a boolean instead of conditionally returning the value or unsupported. useSaveData already functions similarly to this.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@stramel
Copy link
Contributor Author

stramel commented Feb 21, 2020

Rebased and updated to match the hook definitions

@anton-karlovskiy
Copy link
Contributor

@stramel cc @addyosmani

Once we've landed #44, I think we can proceed with this PR.

@stramel
Could you please check my comments?

"source": "index.js",
"typings": "index.d.ts",
Copy link
Contributor

@anton-karlovskiy anton-karlovskiy Mar 11, 2020

Choose a reason for hiding this comment

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

@stramel

It looks duplicated with line 12.
Can we put the type definition file (index.d.ts) inside typings directory, for example, "typings": "typings/index.d.ts"?

@@ -0,0 +1 @@
export function useHardwareConcurrency(): { unsupported: true } | { unsupported: false, numberOfLogicalProcessors: number }
Copy link
Contributor

Choose a reason for hiding this comment

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

@stramel

Could we add semicolon at the end of lines for the purpose of consistency?

@anton-karlovskiy
Copy link
Contributor

anton-karlovskiy commented Mar 11, 2020

@stramel cc @addyosmani

According to typescript handbook, they recommend publishing to the @types organization on npm rather than bundling with the npm package if the package is not written in Typescript.

What are your thoughts on it? :)

@addyosmani
Copy link
Collaborator

Thank you for taking the time to work on typings support for the module and submodules, @stramel. I also appreciate the review here, @anton-karlovskiy.

My personal take is that given the package is not written in TypeScript and we have no short-mid term plans to rewrite it in TS for now, it would make more sense for typings to be published in the @types organization rather than bundling it with this package.

@stramel I think this is a reasonable compromise as it unlocks typings for users that want it. Wdyt?

@stramel
Copy link
Contributor Author

stramel commented Mar 13, 2020

@addyosmani I can do that

@addyosmani
Copy link
Collaborator

Cool. Thank you! With this in mind, I'm going to go ahead and close this PR. Let's follow up with a PR adding a link/note in the README to the typings package once it lands in the types organization? :)

@stramel
Copy link
Contributor Author

stramel commented Mar 13, 2020

Added a draft PR for the typings. I need to clean up the typings and add tests.

Is there anyone who would want to help maintain these types?

@addyosmani @anton-karlovskiy

@stramel stramel deleted the ms/add-typing branch March 13, 2020 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chillin' ❄️ Chillin' (awaiting further feedback)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants