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

have setState work with key, value along with an object #282

Closed
wants to merge 2 commits into from
Closed

have setState work with key, value along with an object #282

wants to merge 2 commits into from

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Aug 20, 2013

This fixes #175 if you're willing to accept it. I'm not sure if that's the best way to document it and I'll gladly make any fixes you want to get it in. :)

This change makes it easier to work with React's API when key is generated programmatically.

@chenglou
Copy link
Contributor

The argument for this is that this fits well into the way lots of other libraries work (e.g. backbone), and frankly I did make the mistake several time at the beginning of using the above format. the argument (more or less) against this is that we could simply put a check and a warning message.

@zpao
Copy link
Member

zpao commented Sep 7, 2013

I think you saw some of our discussions on IRC about this. I think this is totally reasonable to do, we think we might just want to come up with a more explicit API. cc @jordwalke @sebmarkbage

@piranha
Copy link
Contributor Author

piranha commented Sep 10, 2013

Thank you for reminding me about this pull request, I remembered that I promised to write tests for it and got distracted. :-) I'm not sure about API, but this looks like Backbone's Model.set function right now.

@sebmarkbage
Copy link
Collaborator

I think the reason we didn't have this is to avoid problems with key mangling. E.g. in closure compiler.

Previously we didn't have batching of setState commands which also meant that you wanted to pack as many props as possible into a single setState.

I think this a reasonable want for dynamic keys. I'm curious about the usage of dynamic keys though. How are you using this right now? Would it make more sense to use something like a Map abstraction that prevents problems with proto keys and all the other problems using objects as maps?

setState({ myMap: new Map() });

... later

this.state.myMap.set(key, value);
this.forceUpdate();

@piranha
Copy link
Contributor Author

piranha commented Sep 11, 2013

I use it in FormMixin:

exports.FormMixin =
    inputChange: (e) ->
        obj = {}
        obj[e.target.name] = e.target.value
        @setState(obj)

Just thought that in Backbone that was easier. But you are right, that would break compatibility with closure compiler; even in my case it would be better to have a mapping or something...

@piranha
Copy link
Contributor Author

piranha commented Sep 27, 2013

As I think about it more it's better not to have this. :-) linkState from new react removes my need, and I would love to see React working with Closure Compiler advanced mode.

@piranha piranha closed this Sep 27, 2013
@petehunt
Copy link
Contributor

:) also see ReactStateSetters (another internal thing we should probably expose).

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

Successfully merging this pull request may close these issues.

setState (and others) parameters format
5 participants