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

Props and Immutable objects? #2059

Closed
ronag opened this issue Aug 18, 2014 · 10 comments
Closed

Props and Immutable objects? #2059

ronag opened this issue Aug 18, 2014 · 10 comments

Comments

@ronag
Copy link

ronag commented Aug 18, 2014

I've started using immutablejs together with reactjs.

However, I've noticed that passing around immutable objects doesn't quite fit together with reactjs, i.e. {...myObject}, {myObjects.map(object => React.DOM.div()} and propTypes don't quite support them without first converting to JS object which has an unnecessary overhead.

Any thoughts or advice on this issue?

@syranide
Copy link
Contributor

{...myObject} implies cloning (to an empty object), AFAIK, and is not strictly a React issue.

@ronag
Copy link
Author

ronag commented Aug 18, 2014

Well, a cloned immutable object doesn't quite work as props...

None of the issues I mention are strictly an React issue. It's more of a best practice issue, i.e. immutable objects are a best practice (?) (PureRenderMixin) with React. However, React doesn't quite natively support immutable objects.

Maybe something along these lines could be considered in react-future?

@syranide
Copy link
Contributor

The immutable objects advocated by React are simply plain objects that you don't mutate. Using immutablejs should be no problem (AFAIK) with React unless you make the props-object a non-plain object. Which is a general issue with JS, there is no facility for cloning objects, avoid using immutablejs for the props-object and it should be fine?

As for {myObjects.map(object => React.DOM.div()}, sounds like JavaScript iterables or something should be implemented and supported by immutablejs and React.

@chenglou
Copy link
Contributor

We don't have first-class propTypes support for immutable-js, the same way we don't have propTypes support for backbone models. Until immutable stuff are built into JS (es7/8 please!) I think propTypes will have to stay this way, for the better.

@syranide
Copy link
Contributor

@chenglou On another note, I'm curious, would it be imagineable to turn propTypes into an addon/mixin instead? I'm guessing it couldn't use componentWillMount as getDefaultProps would already have been added, perhaps if we expose another life-cycle method? It's not a core React feature and I'm sure a significant fraction don't use it, or have varying preferences.

Anyway, not really pushing for it, it just seems natural to shed things that aren't intrinsic to core.

@davidtheclark
Copy link

I have an idea to share about this. Not a suggested change in the framework, but a solution for using what's there to work with Immutable objects now. (Also curious if others have feedback about what I'm doing.)

I have started attaching an Immutable object to this.props as this.props.improps. (I have a mixin that sets this.improps so it's more easily accessible by methods; and it also adjusts shouldComponentUpdate to take advantage of ref comparison.) So throughout my methods, including render(), I'm accessing values by using get() on this.props.improps (or this.improps) --- so this.props.improps.get('something') instead of just this.props.something.

To type check I can take advantage of the existing custom validator functionality to provide some checks like this:

var Immutable = require('immutable');

function impropTypeCheck(expectedType) {
    return function(props, propName, componentName) {
        var propType = typeof props.improps.get(propName);
        if (propType !== expectedType) {
            return new Error(
                `Warning: Invalid prop '${propName}' of type '${propType}' ` +
                `supplied to '${componentName}'; expected '${expectedType}'.`
            );
        }
    };
}

function impropImmutableInstance(expectedConstructor) {
    return function(props, propName, componentName) {
        if (!(props.improps.get(propName) instanceof expectedConstructor)) {
            return new Error(
                `Warning: Invalid prop '${propName}' supplied to '${componentName}'; ` +
                `expected instance of '${expectedConstructor.name}'.`
            );
        }
    };
}

var impropTypeCheck = {
    impropString: impropTypeCheck('string'),
    impropBool: impropTypeCheck('boolean'),
    immutableList: impropImmutableInstance(Immutable.List)
};

module.exports = impropTypeCheck;

And in my component my propTypes property looks like this:

propTypes: {
    improps: React.PropTypes.instanceOf(Immutable.Map),
    name: impropTypeCheck.impropString,
    label: impropTypeCheck.impropString,
    prompt: impropTypeCheck.impropString,
    value: impropTypeCheck.impropString,
    required: impropTypeCheck.impropBool,
    choices: impropTypeCheck.immutableList,
    errors: impropTypeCheck.immutableList
},

@chenglou
Copy link
Contributor

chenglou commented Dec 3, 2014

Now that Flow is out, guess this issue is less important? Just need to have immutable-js type checked with Flow (@leebyron).

Someone could also make a new mixin a-la propTypes for immutable-js.

@leebyron
Copy link
Contributor

leebyron commented Dec 3, 2014

Yeah, the practice that I've been using is to refer to the Immutable value as a prop:

<MyThing data={immutableMap} />

I usually do similar with state being an object with a single key.

For propTypes I use a technique similar to @davidtheclark's proposal.


Object spread operator (var obj = {...spreadMe}) doesn't work with Maps of any kind, ES6 or Immutable. It's sugar for Object.assign(), and it's usage within JSX is the same:

<div key="foo" ...myProps />
// becomes
React.createElement('div', {key:"foo" ...myProps})
// becomes
React.createElement('div', Object.assign({key:"foo"}, myProps))

I don't think it's likely that props will move from Object to Map anytime soon, but it's certainly possible to create a custom version of JSX transpiler which generates Maps instead of Objects.


Mapping over collections (Immutable or ES6 or otherwise) has been possible in React master branch for some time now and will be included in v0.13

// v0.12, toArray with Immutable collections, not possible with ES6 collections
<ul>{{immutableItems.map(item => <li>{item}</li>).toArray()}</ul>
// v0.13, works fine for all iterables.
<ul>{{immutableItems.map(item => <li>{item}</li>)}</ul>

@syranide
Copy link
Contributor

syranide commented Dec 4, 2014

@leebyron

I don't think it's likely that props will move from Object to Map anytime soon, but it's certainly possible to create a custom version of JSX transpiler which generates Maps instead of Objects.

From my understanding that seems counter-intuitive, isn't props what Objects are intended for? A rather static set of keys, used over and over... whereas Maps are for dynamic lookups?

@gaearon
Copy link
Collaborator

gaearon commented Oct 27, 2016

Closing as this doesn't seem actionable for us.

@gaearon gaearon closed this as completed Oct 27, 2016
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

No branches or pull requests

6 participants