Closed ignatiusreza closed 5 years ago
It's the job of injectIntl
to hoist .propTypes
, and remove the props that it's provided - the consumer should be able to look at the export's .propTypes
and see that intl
is missing - in no way should the consumer be looking at the original .propTypes
in the code and relying on that.
That's correct if we're talking about runtime check done by code.. but, the purpose of this new modifier is to help human in understanding what can (should) be passed in to any components..
If we rely on decorators having to properly hoist and cleanup the propTypes, then for human to understand it, we need to first run the code, and then inspect the component using something like the developer console.. it doesn't sounds like a good use of time from the PoV of developer productivity..
in that case, I’d suggest a comment - if it doesn’t need to be runtime, then it doesn’t need to be a runtime marker in prop-types either.
yes, I thought about that also, and it is indeed the simpler solution.. but, then I thought that having it as built-in support in prop-types
could help in developing a standard in the community so that we can potentially rely on it not just in our own code base, but also when looking into packages or when helping in open source projects..
it also open up the possibility for static analyzer, e.g. to parse and generate list of components inside a project with its props shape.. while it can also be achieved using comments, having it in code comes with a free guard for typo, since if the props are not defined correctly, an error will be raised at runtime..
I think the standard the community follows is runtime detection. If you want to know what a component takes as props, open up a repl, require it, and check its .propTypes. Nothing’s more accurate than that.
hmmm... i'm not so sure about that.. as evidently found in:
where the idea for hoisting propTypes by decorator are rejected vs
where it is accepted.. so there are still no consensus in the community.. and if decorators writer refuse to hoist propTypes, the responsibility to write it correctly and informatively falls once again back to the component being wrapped..
There’s nothing stopping you from manually hoisting the propTypes if the HOC author refuses to do so.
I think an .isInjected
feature in prop-types
itself wouldn't actually solve the problem - it'd have to be in react itself, to avoid warning on missing required injected props, and as such, it's out of scope to solve it only here.
The real solution here, as indicated above, is to make the HOC properly hoist propTypes, and subtract out the props it injects.
Thanks for the PR!
PropTypes
are good in defining what kind ofprops
a component expect to receive, but it doesn't help in communicating whatprops
need to be given when consuming a component, especially when working with a decorated or connected component.The following example shows how the modifier is meant to be used in es6 class. The modifier should also be usable anywhere
PropTypes
can be used.In the example given above, if we don't mark
intl
andlocation
asisInjected
, consumer of the component might think that they will be able to pass in these two props, even though they actually are meant to be injected byreact-intl
andreact-router
respectively. This confusion grows with the number of injected props, as in the case when e.g. usingconnect
fromreact-redux
to inject somestate
anddispatch
..Arguably, we can solve this just by not listing injected props in the
propTypes
.. Unfortunately though, it won't go well if we also have eslint for react/prop-types, since linter will complain if we're using some props that are not defined inpropTypes
..Alternatively, we can also designate a different place to define those injected props, e.g. by having
static injectedPropTypes
.. but, this approach will potentially require changes on react sides..