search

GoogleChromeLabs / react-adaptive-hooks

Deliver experiences best suited to a user's device and network constraints
Apache License 2.0
5.1k stars 113 forks source link

Add typings for module and submodules #21

Closed stramel closed 4 years ago

stramel commented 4 years ago

Adds typings for the main module and submodules.

stramel commented 4 years ago

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

addyosmani commented 4 years ago

Thank you for working on a PR adding typings, @stramel. I see there's already been a request for this in https://github.com/GoogleChromeLabs/react-adaptive-hooks/issues/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.

stramel commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

stramel commented 4 years ago

Rebased and updated to match the hook definitions

anton-karlovskiy commented 4 years ago

@stramel cc @addyosmani

Once we've landed https://github.com/GoogleChromeLabs/react-adaptive-hooks/pull/44, I think we can proceed with this PR.

@stramel Could you please check my comments?

anton-karlovskiy commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@addyosmani I can do that

addyosmani commented 4 years ago

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 commented 4 years ago

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