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

[react-interval] Added typescript typings #131

Merged
merged 3 commits into from
Nov 16, 2017

Conversation

sanex3339
Copy link

Added typescript typings

@@ -41,6 +42,7 @@
"react": "^15.3.0 || ^16.0.0"
},
"dependencies": {
"@types/react": "^16.0.2",
Copy link
Owner

Choose a reason for hiding this comment

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

should it go to dev deps?

Copy link
Author

Choose a reason for hiding this comment

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

This react typings should be installed when other people will install this package.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure but maybe i should replace it to peer dependency

Copy link
Owner

Choose a reason for hiding this comment

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

So probably peerDeps then. We do not use TS and have no plans to, so installing unnecessary dependency does not seem quite right.

Copy link
Owner

Choose a reason for hiding this comment

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

Or even optionalDependencies would be better I reckon
The reason why it is not good to add such things into main deps is that they will live in a project after yarn install --production and will end up in docker images and also in licence reports...

Copy link
Author

Choose a reason for hiding this comment

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

No, optional dependencies is not correct, because React typings is using inside this typings.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm surprised how many people put @types/* into their production deps
https://github.com/search?utf8=✓&q=%27%40types%2Freact%27+extension%3Ajson&type=Code

This is quite sad tbh

@@ -0,0 +1,13 @@
/// <reference types="react" />
Copy link
Owner

Choose a reason for hiding this comment

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

this file needs to be added to files section of package.json, otherwise it will not be included into a package.

To check that locally you can run yarn pack (or npm pack) inside folder and then check contents of resulting react-interval-*.tgz

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Owner

@nkbt nkbt left a comment

Choose a reason for hiding this comment

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

Great, thanks 🙏

@nkbt nkbt changed the title Added typescript typings [react-interval] Added typescript typings Nov 16, 2017
@nkbt nkbt merged commit 4c31d56 into nkbt:master Nov 16, 2017
@sanex3339
Copy link
Author

When i can expect release with this typings?

@vkhv
Copy link

vkhv commented Nov 20, 2017

@nkbt ?)

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.

3 participants