Skip to content
This repository has been archived by the owner on Sep 1, 2024. It is now read-only.

How to depend on this package? #4

Closed
no23reason opened this issue Apr 8, 2017 · 11 comments
Closed

How to depend on this package? #4

no23reason opened this issue Apr 8, 2017 · 11 comments

Comments

@no23reason
Copy link

Hi,
I have a package that has react as a peerDependency and provides PropTypes for the component it exports. Now that the prop-types is a separate package I would like to use it as a peerDependency as well but in an opt-in way (for users that use TypeScript, PropTypes are redundant). Is there a recommended way of handling this situation?

Thanks

@gaearon
Copy link
Contributor

gaearon commented Apr 9, 2017

Is there any problem with just using it as a dependency?

@bjrmatos
Copy link

bjrmatos commented Apr 9, 2017

@gaearon perhaps something like the following could be a problem?

if multiple libs are using prop-types as a dep, would not this mean that the code of prop-types would be repeated several times in a bundle?

context: https://twitter.com/bjrmatos/status/850590087364489216

@no23reason
Copy link
Author

Exactly as @bjrmatos says, it is about optimising the bundle. I like the idea of separating the PropTypes stuff (I personally don't use it in my apps as I have TypeScript), but I am afraid this could lead to duplication of its code across the bundle. What I would like to achieve is this:

If the prop-types is available in the bundle use it. The user must have explicitly put it there so we can assume they want to use the PropType checks.
Otherwise ignore them altogether thus reducing the bundle.

I'm not sure how to do this though (if it is even possible)...

@gaearon
Copy link
Contributor

gaearon commented Apr 9, 2017

If everyone puts ^ in the version then no, both npm and Yarn should deduplicate it. Now, in practice unfortunately this doesn’t always happen in npm 2, but it’s not even in Node LTS so I think it should be okay.

@mariuslundgard
Copy link

mariuslundgard commented Apr 9, 2017

There's no documentation of how to import and use prop-types neither in this repo nor in the React docs (although pretty straightforward to figure out). I presume:

import PropTypes from 'prop-types'

(Sorry if this is the wrong place for this comment.)

Also, would perhaps be easier to figure out if this package was named react-prop-types?

@gaearon
Copy link
Contributor

gaearon commented Apr 9, 2017

Apologies for that. We released on Friday in a hurry and missed some issues like this. We will add documentation soon but for now please see examples in our blog: https://facebook.github.io/react/blog/2017/04/07/react-v15.5.0.html

@sodik82
Copy link

sodik82 commented Apr 9, 2017

@gaearon is it possible to write library that uses PropTypes in a manner that it is compatible with both e.g. React 15.4 and 15.5. (without warnings of course)? Or alternative question - is it possible to use prop-types package with older React (let's say 15.0) in a way that prop type checking would work?

@bjrmatos
Copy link

bjrmatos commented Apr 9, 2017

If everyone puts ^ in the version then no, both npm and Yarn should deduplicate it. Now, in practice unfortunately this doesn’t always happen in npm 2, but it’s not even in Node LTS so I think it should be okay.

i think that putting ^ in a prop-types dependency will be something easy to miss while developing third-party components. (since i think many people are using the --save-exact flag in npm config)

from the React 15.5 blog post:

The biggest change is that we've extracted React.PropTypes and React.createClass into their own packages. Both are still accessible via the main React object, but using either will log a one-time deprecation warning to the console when in development mode. This will enable future code size optimizations.

the important part is "This will enable future code size optimizations."..

i think that in the end there will be no code size optimizations at least for apps (for the source of React, sure) if there is easy way that the code of prop-types gets duplicated in every component dependency.

but i think this is something that can be solved with education, maybe a section in the README of this package and in the React's docs (prop types section) could help?.

Considering that PropTypes in React plays a big role in developer experience i think that this problem should not be taken lightly

@gaearon
Copy link
Contributor

gaearon commented Apr 9, 2017

I totally see (and agree with) your point.

I don't think "future optimizations" referred to PropTypes specifically. You are right it doesn't enable any optimizations, and in bad cases can make the issue worse. I think @acdlite was referring to createClass removal in this sentence which does allow us to drop some weight. The splitting of prop-types had more to do with organizational concerns (e.g. we don't actively maintain them, issues about them get lost in React repo, they are useful beyond React).

We could've split prop-types into a library but still reference it from the React object which would solve the problem you're describing. So that's one possible option. I'm not sure how the team feels about this.

We're going to discuss this with the team more on Monday.

@bjrmatos
Copy link

bjrmatos commented Apr 9, 2017

We could've split prop-types into a library but still reference it from the React object which would solve the problem you're describing. So that's one possible option. I'm not sure how the team feels about this.

We're going to discuss this with the team more on Monday.

thnk you, i'm sure there will be something to do to tackle this problem, your solution seems the best to me but let's see. 😄

@gaearon
Copy link
Contributor

gaearon commented Apr 11, 2017

Documented the suggested approach in e681b40.

More on motivation for this change: #21 (comment).

i think that putting ^ in a prop-types dependency will be something easy to miss while developing third-party components. (since i think many people are using the --save-exact flag in npm config)

We don’t expect this to be a huge issue, and we’re going to give this a try. We can reconsider this before 16 if the problem turns out widespread.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants