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

Support Symbol keys for props #7552

Closed
zemlanin opened this issue Aug 24, 2016 · 26 comments
Closed

Support Symbol keys for props #7552

zemlanin opened this issue Aug 24, 2016 · 26 comments

Comments

@zemlanin
Copy link

Do you want to request a feature or report a bug?
I want to report a bug

What is the current behavior?
render() doesn't receive props with Symbol keys (for example, {[Symbol()]: 'lol'}). I guess it is because of hasOwnProperty in ReactElement.createElement

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via jsfiddle or similar.
https://jsfiddle.net/sh2xbm3x/1/

What is the expected behavior?
Symbol-keyed props passed to render()

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
Every single one, as far as I know

@gaearon
Copy link
Collaborator

gaearon commented Aug 24, 2016

Can you please explain your use case for this?

@zemlanin
Copy link
Author

Framework-specific props

For example, brabadu/tanok (internal implementation of elm-architecture) uses convention with tanokStream (or deprecated eventStream) property name to pass around Rx.Subject for sending actions. Instead, it can export Symbol to mark a component as framework-dependent without inventing names to avoid collisions

I guess, Symbols can find place in acdlite/recompose, but I didn't use it that much

Isn't a possibility of receiving Symbol-keyed props enough of a reason to process them in some way:

  • passing it to an element
  • throwing a warning
  • mentioning this behavior in the documentation

?

@jimfb
Copy link
Contributor

jimfb commented Aug 24, 2016

A framework could also choose a cryptographically random 64-character alphanumeric key, which would effectively eliminate any possibility of a name conflict. Save it to a variable and use it as a symbol, if so desired.

Another workaround is to create a prop called frameworkProps, which takes in a general object containing your framework-specific props. Then you can use symbols and anything else you desire. Usage would be: <YourComponent frameworkProps={{[Symbol()]: 'lol'}} />

The simplicity of "keys are strings" is something that I'm a little reluctant to give up. If component authors start accepting symbols as keys, things become more complicated. For instance, what is the story around JSX?

My intuition is that if we do anything, perhaps it should be to throw a warning if we encounter a symbol as a prop key.

@zemlanin
Copy link
Author

Random string key is a fine alternative, but it has the same "overloaded" look in JSX as a symbol does:

import { randomKey } from 'framework'

<Component {...{[randomKey]: 'lol'}} />

@zemlanin
Copy link
Author

Replaced passing Symbol props to a component with warning about react's behavior in #7554

@clarabstract
Copy link

clarabstract commented Oct 27, 2016

I'd like to argue in favor of @zemlanin's use case. The very purpose of Symbol is to be a private property identifier that cannot collide with another - it feels like a much more appropriate solution to this sort of problem compared to randomKey.

I think the more useful simplifying guideline would be "react prop keys are just like object prop keys" (rather than just "keys are strings"). For the most part they are exactly object keys, and when dealing with JSX you can use the spread operator the same way you do with objects. There is no slippery slope towards supporting all manner of exotic types as prop keys, any more than there is for object keys. The valid key types are simply: string, Symbol - end of list.

React has tried to stick close to ES6+ in terms of style so far, it would be nice to continue along those lines by treating Symbols the same way ES6 does - i.e. collision-proof private key identifiers, for use by frameworks and extension mechanisms, never iterated over, but usually copied (e.g. by Object.assign, and so in turn perhaps by React.cloneElement).

In terms of JSX syntax implications, again, I don't see anything special that would need to be done here. Props are always strings, so you wouldn't be able to use symbols there, it would either have to be as part of the spread object, or with cloneElement. If in the future you allow prop name expressions using a square bracket syntax (e.g. <Component [propName]={value} /> then that would, of course, also cover symbol prop names, but that's just incidental.

Random prop names or long namespaced names like __myFramework__propName are good enough solutions too, but Symbol just seems like a cleaner fit, much like like the move from createClass to plain ES6 classes.

@cheapsteak
Copy link

Another use case:

acdlite/recompose#358

const namespace =  (ns, ...hocs) => compose(
  withProps(props => ({ $parentProps: props })), // TODO $parentProps as symbol
  ...hocs,
  mapProps(props => ({ [ns]: props, ...props.$parentProps }))
)

@smrq
Copy link

smrq commented Sep 7, 2017

Yet another use case.

https://reacttraining.com/react-router/web/guides/dealing-with-update-blocking

<UpdateBlocker location={location}>
  <NavLink to='/about'>About</NavLink>
  <NavLink to='/faq'>F.A.Q.</NavLink>
</UpdateBlocker>

In react-router v4, the suggested practice for ensuring that pure components have their impure react-router children update is to pass an otherwise unused location prop into the pure component. Since this prop is only used for the purposes of shouldComponentUpdate(), it would be nice to be able to use a Symbol and guarantee that the name will never, ever conflict with the real props for any given pure component.

const props = {
  ...
  [Symbol('location')]: location
};
<UpdateBlocker {...props}>
  <NavLink to='/about'>About</NavLink>
  <NavLink to='/faq'>F.A.Q.</NavLink>
</UpdateBlocker>

This was one of those things that I tried, only to be mildly surprised that it didn't work. (In my case the syntax was cleaner than above, because the prop was being injected via react-redux connect().)

I think the more useful simplifying guideline would be "react prop keys are just like object prop keys"

...completely nails it, in my opinion.

@gaearon gaearon changed the title Lost props with Symbol keys Support Symbol keys for props Oct 4, 2017
@mmiller42
Copy link

mmiller42 commented Dec 17, 2017

Another use case is to create non-enumerable props.

Take, for instance, a component that is wrapped in a container that injects additional props in (dispatch from react-redux is a good example).

A common React convention is to use the spread operator to select specific props and pass the rest down, like so:

render() {
  const { backgroundColor, ...restProps } = this.props
  return (
    <div style={{ backgroundColor }}>
      <ChildComponent {...restProps} />
    </div>
  )
}

This would inadvertently pass dispatch down to ChildComponent, something that is rarely deliberate. Symbols, however, are inherently non-enumerable, so if dispatch were a Symbol (not necessarily a default behavior, but certainly one that could be configured by connect(), then all uses of the dispatch prop would have to be explicit.

And it would certainly be much more natural to use this.props[dispatch](action) (where dispatch is a Symbol imported from react-redux) than it would be to use a randomly generated identifier to avoid collisions.

@gaearon
Copy link
Collaborator

gaearon commented Jan 2, 2018

It sounds to me like this is just trying to work around the real problem (HOC pattern pollutes props) with a solution that further obscures it.

We are looking at revamping the context API in a way that doesn’t present this problem. Would this help you?
reactjs/rfcs#2

@mmiller42
Copy link

mmiller42 commented Jan 2, 2018

Ah, yeah, this is cool. I didn't want to use context because the current API didn't really seem much better than props, since it still deals with naming collisions and such. Nice proposal! 👍

@ruyasan
Copy link

ruyasan commented Jan 3, 2018

It's a lovely proposal (I am 👍 for the proposal itself), but not all uses of Symbol props translate to using contexts instead. Sometimes you want the locality actual props provide. Most use cases here have something to do with components that are intentionally coupled with each-other and meant to be used together as a coherent set. Sometimes this works best with contexts (e.g. forms and their fields) but other times, the communication really has to happen between parent and direct children only. E.g. the way <select> and <option> work - or more generally, when we use components as a mechanism for passing specific data to specific parent components. Going the other way as well: Sometimes parents need to inject specific data, privately, into specific children - e.g. when you have a parent that needs to do facilitate some coordination between its children. This came up for me personally for custom layout systems, but I'm sure there are broader examples.

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

We have some very rough ideas about solving these use cases too without the notion of private props.

@ruyasan
Copy link

ruyasan commented Jan 5, 2018

Oh heck yeah, that would be much better.

Between the context proposal and the createReturn one, yeah, I believe that covers every use case I've ran into.

@nathggns
Copy link

The call return API previously mentioned was never finalised and has since been removed. There are times where library authors may want to attach pass private values through levels of consumer components via props. Is there any serious reason not to support Symbols in props?

@stale
Copy link

stale bot commented Jan 10, 2020

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.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@cameron-martin
Copy link

Is there any info available on why call return was scrapped?

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Apr 9, 2020

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 additional information, please include with in your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2020
@nathggns
Copy link

nathggns commented Apr 9, 2020

bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2020
@thomassuckow
Copy link

One issue I am having is the Context API doesn't seem to have a way to know the order of components. So that leads to using a parent child set of components where the parent has to tell the child its index via cloning children.

@stale
Copy link

stale bot commented Jul 26, 2020

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!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 26, 2020
@zemlanin
Copy link
Author

bumpity bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 26, 2020
@LinusU
Copy link
Contributor

LinusU commented Sep 10, 2020

Just ran into this today, was a bit surprising that it just got silently stripped 🤔

My use case was to pass internal props down to specific children, in this case the background color to the Row in a Table component:

import React, { ReactElement } from 'react'
import { View } from 'react-native'
import { HStack, VStack } from 'react-stacked'

const kBackgroundColor = Symbol('BackgroundColor')

interface CellProps {}

export const Cell: React.FC<CellProps> = ({ children }) => (
  <View style={{ flexBasis: 1, flexGrow: 1, flexShrink: 0 }}>{children}</View>
)

interface RowProps {
  [kBackgroundColor]?: string
  children?: ReadonlyArray<ReactElement<CellProps>> | ReactElement<CellProps> | null
}

export const Row: React.FC<RowProps> = ({ children, ...props }) => (
  <HStack backgroundColor={props[kBackgroundColor]}>{children}</HStack>
)

interface TableProps {
  children?: ReadonlyArray<ReactElement<RowProps>> | ReactElement<RowProps> | null
}

export const Table: React.FC<TableProps> = ({ children }) => (
  <VStack>
    {React.Children.map(children, (child, index) => (
      React.cloneElement(child, { [kBackgroundColor]: ['#fff', '#eee'][index % 2] })
    ))}
  </VStack>
)

I guess I could create a context for each row, but my gut feeling is that performance will suffer 😅


edit:

My primary use case for this is that I don't want my internal props to show up in autocompletion, and I'm using TypeScript. With those two prerequisites I found this workaround that I think is acceptable:

const kBackgroundColor: unique symbol = '_backgroundColor' as any

This will treat kBackgroundColor as a unique symbol and thus not display that there is a prop called _backgroundColor on <Row>, yet I can still pass it wherever I have access to kBackgroundColor.

@stale
Copy link

stale bot commented Dec 25, 2020

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!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Dec 25, 2020
@zemlanin
Copy link
Author

bümp

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Dec 25, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

I don't think we should support this.

The purpose of props is to be explicit about which data gets passed down. If you want to make it more implicit and "secret" between two components, you can use Context. Patterns that rely on cloneElement are in general last resort and not recommended because you can't put a component in the middle — they break composition. So it seems like every time you want to do this, there's a better way to do it that's more idiomatic.

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

No branches or pull requests