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

setState (and others) parameters format #175

Closed
chenglou opened this issue Jul 7, 2013 · 8 comments
Closed

setState (and others) parameters format #175

chenglou opened this issue Jul 7, 2013 · 8 comments

Comments

@chenglou
Copy link
Contributor

chenglou commented Jul 7, 2013

Currently accidentally using this.setState('title', 'Car') throws an unhelpful MERGE_CORE_FAILURE error. Would be nice to either throw a better error or accept this format.
Same goes for setProps, etc.

@petehunt
Copy link
Contributor

petehunt commented Jul 7, 2013

Are you using the minified React package or unminified?

@chenglou
Copy link
Contributor Author

chenglou commented Jul 7, 2013

minified, 0.3.3
Here's the error message for unminified:
Critical assumptions about the merge functions have been violated. This is the fault of the merge functions themselves, not necessarily the callers. react.js:4651

@petehunt
Copy link
Contributor

petehunt commented Jul 7, 2013

Can you try with unminified? You should get a more helpful error message. If not, we have a bug.

@petehunt
Copy link
Contributor

petehunt commented Jul 7, 2013

OK so that error message still sucks, but at least you're getting messages in the correct build. Thanks for the report!

@tomocchino
Copy link
Contributor

@jordwalke will have some thoughts on this, since the proposed API doesn't support GCC key crushing, or whatever. I could go either way.. this API is more convenient and prevents an object allocation in the simple case (and it's familiar), but it also makes this function a bit more flexible, and I'm typically from the "constraints are liberating" camp.

@chenglou
Copy link
Contributor Author

Just to clarify @tomocchino, I brought this habit over from Backbone and other libraries. I'm also in the same camp as you, and accepting this format was a mere idea. I'd much prefer a simple error message in the line of you need to pass an object for…, and then get over with this once and for all.

@piranha
Copy link
Contributor

piranha commented Aug 20, 2013

I would like to see setState accept both variants (either an object or a key with a value), it could look like that then:

  setState: function(key, value, callback) {
    var partialState;
    if (typeof key === 'string') { // {string, anything, fn}
        partialState = {};
        partialState[key] = value;
    } else { // {object, fn}
        partialState = key;
        callback = value;
    }

    // Merge with `_pendingState` if it exists, otherwise with existing state.
    this.replaceState(
      merge(this._pendingState || this.state, partialState),
      callback
    );
  },

I don't know if that (having two sets of arguments in a function) is acceptable style for React, but it is definitely used quite a bit in JS world. If that's acceptable, then another problem arises - how do I document that so that it renders nicely in documentation?

@zpao
Copy link
Member

zpao commented Oct 28, 2013

Based on our lack of movement on this and some of the discussion in the associated PR, I'm going to wontfix this. If it comes up again though I'm open for discussing further.

@zpao zpao closed this as completed Oct 28, 2013
bvaughn pushed a commit to bvaughn/react that referenced this issue Aug 13, 2019
Assign timeoutID to avoid multiple requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants