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

Create typings.d.ts #76

Merged
merged 5 commits into from
Aug 30, 2018
Merged

Create typings.d.ts #76

merged 5 commits into from
Aug 30, 2018

Conversation

edorivai
Copy link
Collaborator

This is the TypeScript typing that we're using internally. Please let me know if changes are required.

Copy link

@swese44 swese44 left a comment

Choose a reason for hiding this comment

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

this will be nice to have included with the npm module, thanks!

typings.d.ts Outdated
@@ -0,0 +1,9 @@
declare module 'react-media' {
interface MediaProps {
query: string;
Copy link

Choose a reason for hiding this comment

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

query should be a string or an object with string keys and values of number, string or boolean. looks like react-media will pass a string query to window.matchMedia(query), and will pass an object query directly to json2mq:
https://github.com/akiran/json2mq

maybe this:

export interface MediaQueryObject {
    [id: string]: boolean | number | string;
}

export interface MediaProps {
    query: string | MediaQueryObject;
    ...
}

Copy link

Choose a reason for hiding this comment

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

looks like query can also accept an array of objects, so maybe this is better:

export interface MediaQueryObject {
    [id: string]: boolean | number | string;
}

export interface MediaProps {
    query: string | MediaQueryObject | MediaQueryObject[];
    ...
}

typings.d.ts Outdated
interface MediaProps {
query: string;
defaultMatches?: boolean;
children: ((matches: boolean) => JSX.Element) | JSX.Element
Copy link

Choose a reason for hiding this comment

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

children should be optional, and i believe these should specify React.ReactNode instead of JSX.Element

typings.d.ts Outdated
query: string;
defaultMatches?: boolean;
children: ((matches: boolean) => JSX.Element) | JSX.Element
}
Copy link

Choose a reason for hiding this comment

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

we're also missing the optional render prop:
render?: () => React.ReactNode;

typings.d.ts Outdated
@@ -0,0 +1,9 @@
declare module 'react-media' {
interface MediaProps {
Copy link

Choose a reason for hiding this comment

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

don't forget to export the props interface so other components can access it

@edorivai
Copy link
Collaborator Author

@swese44 Thanks for all the hints! I think I've fixed all of the issues.

@mjackson
Copy link
Member

If you'd like to include this in the npm package you'll need to add it to the files array in package.json.

@edorivai
Copy link
Collaborator Author

I've renamed the TS file to index.d.ts, and added it to the files array. Hope everything is set up properly!

One final question: the typings now depend on @types/react being installed as well, as it uses the global React.ReactNode and React.Component types. I was wondering if anyone knows the best practices around dependencies between TS typing files. I don't really have any experience with this.

index.d.ts Outdated
@@ -0,0 +1,14 @@
declare module 'react-media' {

Choose a reason for hiding this comment

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

This line is unnecessary when you bundle typings with the package like we're doing here. The module name will be the one in package.json.

package.json Outdated
@@ -8,7 +8,8 @@
"files": [
"cjs",
"esm",
"umd"
"umd",
"index.d.ts"

Choose a reason for hiding this comment

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

You seem to be using tabs, but the rest of the file doesn't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, my bad

@unindented
Copy link

@edorivai left two comments.

Regarding dependencies, you should import them in your index.d.ts file:

import { Component, ReactNode } from 'react';

@edorivai
Copy link
Collaborator Author

@unindented Fixed both issues, and converted index.d.ts to spaces as well 🤓

@unindented
Copy link

The line declare module 'react-media' is unnecessary in this instance, cause you're bundling the types with the module itself.

@edorivai
Copy link
Collaborator Author

Oh really, didn't know that. Nice!

@edorivai
Copy link
Collaborator Author

Btw, once everything looks okay, I'll happily squash everything into 1 commit, if you guys prefer that.

@farzadmf
Copy link

farzadmf commented Apr 5, 2018

Hi,

Is this going to be merged? Is there a workaround to use the library in Typescript?

Thanks

@edorivai
Copy link
Collaborator Author

edorivai commented Apr 5, 2018

@farzadmf workaround would be to copy/paste this file into your project:
https://github.com/edorivai/react-media/blob/37aa81c350eac95bc2d8779400959abbdf87767b/index.d.ts

@farzadmf
Copy link

farzadmf commented Apr 5, 2018

Thank you @edorivai , I thought of that 👍 ; just wanted to check if there's a "more official" way to do it :)

@unindented
Copy link

@mjackson any chance of getting this merged, and a new release published?

@mjackson mjackson mentioned this pull request Aug 25, 2018
@edorivai edorivai merged commit 9026956 into ReactTraining:master Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants