-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Feature request: renderTypes #10020
Comments
If I understand this correctly, you could do most of this (i.e. annotating components with Also, what might |
Yes, exactly - without React enforcing it, it's just adding an arbitrary convention, which imo is potentially very dangerous for the ecosystem. I'm not sure what it would look like with fragmented rendering; but I'd assume that fragments would be able to optionally specify renderTypes as well if they wanted - there's nothing inherent about the feature as proposed that would require a single root node. In other words, if something rendered |
cc @sebmarkbage |
I really want this for our Flow support since Flow can infer it all the way down and ensure. I haven't considered this for PropTypes. We might be able to do just what you want here but, if not, coroutines in Fiber does let you implement this kind of thing in user space and is the wider solution for this kind of parent/child relationships with indirections. |
It'd be very valuable to use to be able to use it pre-Fiber, so hopefully this is a feature that can be added easily :-) |
We most likely won't back port this (or most other new features) to React 15. Get on the Fiber train. :) |
How's server rendering of Fiber coming? :-p |
It's landed. |
Passes all the tests so it must be done. :) |
Regardless tho, it's also pretty disappointing that prior to v16 being released, a new feature for 15 wouldn't be considered. Would you be willing to accept it if I wrote up the PR? |
We're getting started on the release work for 16 now. We'd have to switch focus from that to do another 15 release (with everything that entails to try to not break the ecosystem). It's possible we could do that if you think it's worth while but I'm not even sure how to correctly implement this in 15. Would be much easier in Fiber actually. The reason is that in 15 we don't always have the ability for the parent to see all possible children. A deep setState on |
It'd be enforcing what it actually returns, not what it theoretically could return - so you wouldn't get a renderType warning on |
Releasing 15.5 and 15.6 took several weeks of effort from several people. We are not in a good position to add any more features to 15.x at this point. The branch has diverged way too much, and it’s causing much duplicated effort. We also don't have the usual stability guarantees for it anymore because we don't use 15.x in production. |
Chiming in kind of late here but did you consider something like this? const Test = props => <div>Example</div>;
Test.propTypes = {
prop: function(props, propName, componentName) {
if (!recursiveTypeCheck(props[propName], 'strong')) {
throw Error('Nooo!');
}
}
};
function recursiveTypeCheck(element, type) {
if (element.type === type) {
return true;
} else {
return React.Children.toArray(element.props.children)
.some(child => recursiveTypeCheck(child, type));
}
} Edit Disregard! I believe the missing case is for a class/functional component that renders |
Hi @ljharb - the React team talked about this issue and we came up with some concerns.
*Here's some more detail about what we considered: * We also considered the idea that this could be implemented outside of React - Sebastian commented earlier that
It could be that, as you said,
That said, at this point we have moved Hope this is helpful and please let me know how this sounds and if there is anything we can clarify. Thanks also for bringing this issue to my attention, so I could bring it up again with the team. |
Thanks for the response! While I think a module outside of React is definitely a better way to go about it; there's still dev-only hooks into
I'm not sure why - |
To clarify (this was my objection): you might have a Button component that takes buttonStyle. Maybe you have a ButtonGroup that does If you do pass a FancyButton instead (and FancyButton renders to Button), ButtonGroup will clone the FancyButton element and add buttonStyle prop to FancyButton instead. But FancyButton doesn't necessarily take buttonStyle; it could take a totally different set of props. (If typechecks before cloneElement aren't the primary value you see, I'm curious what you see as the value proposition for enforcing this.) |
We tend to avoid So, |
But so why does ButtonGroup care if FancyButton returns a Button? Having that guarantee allows it to do what that it can’t otherwise? |
The only application I can easily see here is helping enforce CSS constraints. e.g. if only I don’t yet see where else that would be useful since the rendered child is opaque and you can’t interface with it directly. |
@sophiebits @gaearon It's a design consideration. We don't want devs to be able to freely put whatever they want, wherever they want. By restricting ButtonGroup to only have Buttons, then devs can't naively stick a dropdown menu or something in a ButtonGroup. A perhaps more common pattern for us is: we have a propType called "textlike". It allows a string, or a |
It's subtyping vs nominal typing. Sometimes it makes sense to add some restrictions that won't technically fail but will organizationally fail. |
Yes, but what bad happens if you do? :) I'm fine if the answer is "nothing, this is primarily for documentation enforced by code", just want to make sure I'm not missing anything.
If I'm authoring a component, when/why would I want to restrict my children to allow all kinds of Text but no other components? This seems further from the CSS argument even since a Heading likely has a very different appearance from any other text. |
The bad thing is "the interface is inconsistent, and designers (and without realizing it, users) are sad". It's got nothing to do with CSS; we want our entire codebase as restricted as possible, and we heavily use custom children propTypes to do it already. If non-text is allowed, people (devs/engs/PMs) will put only icons there (which isn't accessible), or will abuse existing styling to use containers for layout (which often breaks on various responsive breakpoints), or will employ any of a multitude of subpar UX patterns without realizing it. We want nothing to be permitted except known-good patterns, and renderTypes (or anything that provides similar functionality) will help close some of the few remaining loopholes/awkward surfaces in this approach. |
I’m a bit worried that if we add this, it will turn into a cargo cult. People will be adding |
That's a fair criticism; this is just the solution that made sense to me. If there's another way to meet the use case that would be more obvious/intuitive, I'm all for it! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
Bump. |
Not suggesting that this API needs to be in React, but what about flipping the API around to validate it from the parent? function isButton(element) {
return true || false
}
function ButtonGroup({ children }) {
return (
<div className="btn-group">
<Assert is={isButton}>{children}</Assert>
</div>
)
} |
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
bump |
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
eternal bump |
I’m going to close this. It seems that the web dev industry has settled on TypeScript for people who want type checking in their code. Static type checks (as I’m sure you know) have the advantage that they detect problems at build time even if the faulty code isn’t exercised during development. At this point I see it as much more likely that in the future React removes propTypes support rather than adding renderTypes. You can also write a custom linter if this is a concern specifically for your employer. It’s not a common enough request that it makes sense to add to React now. |
Fair enough, but that’s unfortunate, since typescript is both unsound and not a superset of JS, and can’t ever approach the validation power propTypes offer. The benefit of this proposal is that it’s a convention for the (not small) part of the community that still uses propTypes to organize around - the goal wasn’t just for a convention inside a company, it was for the broader ecosystem as well - something only React can define. |
Do you want to request a feature or report a bug?
Request a feature
Per some discussion today with @tomocchino and @thejameskyle, I'd like a non-Flow mechanism to annotate what type(s) of elements a component expects to render.
Here's some examples, with Flow types for comparison (that I realize may not be currently checked in Flow, yet):
Inside @airbnb, we have lots of use cases where we have container components in a separate package - say, a
<ButtonRow>
, and we have intentionally restrictive propTypes on itschildren
prop, to only allow aButton
(also in the same package). However, in an app that consumes this component library package, a dev may want to create a<SpecialProductButton />
that in turn renders a<Button>
- however, they're unable to pass it intoButtonRow
(our propType warnings fail tests), even though conceptually it should be permitted.Having
.renderTypes
would allow us to widen ourchildren
propType to allow for either a<Button>
, or anything that renders a<Button>
, which helps us maintain separation of concerns (the package doesn't have to know about<SpecialProductButton>
to accept it) as well as maintain strictness (the package doesn't have to allow any wacky element inside<ButtonRow>
).I imagine the implementation to be:
.renderTypes
exists on the componentelementType
(...Component.renderTypes)({ children: renderedValue }, 'children', ...)
,(cc @spicyj)
The text was updated successfully, but these errors were encountered: