magalhas / backbone-react-component

A bit of nifty glue that automatically plugs your Backbone models and collections into your React components, on the browser and server
http://magalhas.github.io/backbone-react-component/
MIT License
810 stars 72 forks source link

Incoming on v0.8.0 #55

Closed magalhas closed 9 years ago

magalhas commented 9 years ago

Major

Minor

Bug fixes

rzhou186 commented 9 years ago

Does the switch to setState mean that for child components, we won't need to remount them as new root components and they'll still retain their data bindings?

magalhas commented 9 years ago

Yes :+1:

rzhou186 commented 9 years ago

That's fantastic! The README will need to be updated too, then...

magalhas commented 9 years ago

Everything will need an update, which is great but also scary because I don't have that much free time. I hope no one gets mad about having to refactor their code, I mean, I finally agree that using @state is more viable then @props and I think that's a general feeling, Also it improves the way of creating an entire application without going Flux :)

rzhou186 commented 9 years ago

This is an extraordinary library and has been a huge help to all of us. Thanks magalhas!

magalhas commented 9 years ago

@rzhou186 feel free to try out the master, all changes are already in place. I still have to update all the docs and demos to release a beta version of it, though feedback would be awesome meanwhile.

ieugen commented 9 years ago

Any ETA on this? I wish to use it in a project I'm migrating to React and would like to avoid migrating from 0.7.2 to 0.8. How stable is the alpha version from master?

magalhas commented 9 years ago

The latest alpha version doesn't include the latest changes. I would recommend you to use the HEAD of master. I'll release between today/tomorrow a v0.8.0-beta to make it easier. I think the HEAD is already pretty stable but I need more feedback to be sure. Anyway I don't intend to make any API changes in the future.

ieugen commented 9 years ago

Cool. I'll be your guinee pig then. I'm allready using master via bower. Let me know you push the beta so I can follow and report broken things. Cheers,

magalhas commented 9 years ago

Thank you @ieugen. Please let me know if you're having any issues so far. Before the beta release I need to update the documentation and demos, it won't take me more than an hour I guess.

magalhas commented 9 years ago

@ieugen I've just tailed v0.8.0-beta.1 release. There are still some examples/documentation changes missing, but that's it. Can you let me know your experience so far? Thanks.

ieugen commented 9 years ago

I've just pulled from master and manually tested the application. No issues so far, great job. I will implement some other features in the next few days and get back if there are things to report.

ieugen commented 9 years ago

No issues so far on my side. Doing some basic stuff only. Great job.

One thing that I noticed, is that under the model is available under this.props.model as a Backbone model and under this.state as a JSON object. Is any of this going to change (from the top post in this thread I understand that model will not be available under props )?

magalhas commented 9 years ago

The model instance reference will be available under @props, though the JSON representation of it will be living under @state.

Before this patch the model reference was removed from @props and the JSON representation could be found on it.

One thing that I'm not sure that should be changed is a model JSON representation maybe should live inside @state.model instead of directly into @state.

What do you think?

ieugen commented 9 years ago

It's quite debatable, but I think that namespacing them under stat.model is good. How do you currently handle issues when the view uses it's own state via getInitialState() and you have a name clash with the model attributes?

magalhas commented 9 years ago

Model properties will replace @state properties if they collide. That's the main reason I think nesting model properties under @state.model is a good choice. I think I'll include that change in this patch.

ieugen commented 9 years ago

Yes, it makes sense.

hardchor commented 9 years ago

Is anyone else experiencing that componentWillReceiveProps isn't firing anymore since this change?

P.S.: For clarification, my code:

    getInitialState: function () {
        return {
            someState: true
        }
    },

    componentWillMount: function () {
        this.setState({someState: this.props.model.user.getSomeState()});
    },

    // used to get fired when the model changed, but isn't anymore
    componentWillReceiveProps: function (nextProps) {
        this.setState({canBuild: this.props.model.user.getSomeState()});
    },

P.P.S.: Looking further through the recent changes, I've noticed that props has been completely replaced with state. Shouldn't both be kept in sync when an upstream model changes?

magalhas commented 9 years ago

Props contains the model/collection references passed from above and state contains the JSON representation of it. Yes, they're always in sync.

hardchor commented 9 years ago

Are you absolutely sure about this? https://github.com/magalhas/backbone-react-component/blob/master/lib/component.js#L233 seems to suggest it only updates the state, which wouldn't trigger componentWillReceiveProps.

magalhas commented 9 years ago

componentWillReceiveProps only gets called if there are changes to props. If a collection or model updates, props doesn't change because the reference is exactly the same.

What you should do is replace componentWillReceiveProps with shouldComponentUpdate or componentWIllUpdate.

hardchor commented 9 years ago

You can't (or shouldn't) update state from those methods(as the docs state and my own tests confirm).

I've relied on componentWillReceiveProps in the past to notify me of model changes. I think the best way to go for now is to subscribe to model.on('change' ...) in componentWillMount and update someState from there.

What do you think? Muito obrigado ;)

magalhas commented 9 years ago

I'm almost sure that mutating state in @shouldComponentUpdate is not an anti-pattern, but in @componentWillReceiveProps it's.

Regarding your suggestion it looks like a ugly trick because the intention of this lib is to spare you from doing that.

De nada ;)

hardchor commented 9 years ago

I don't think it is, it's even stated as an explicit example in the docs for componentWillReceiveProps (http://facebook.github.io/react/docs/component-specs.html#updating-componentwillreceiveprops).

shouldComponentUpdate does get triggered on props and state updates, but is only meant to be used for validation, so updating the state (if at all possible, which I'll check in a minute) feels more like a hack.

I do agree that my proposed solution is a bit of a hack as well. Do you think we might be able to manually trigger componentWillReceiveProps when we know that the model has changed, e.g. in onSync?

magalhas commented 9 years ago

@hardchor I've just created an issue in React's repository with the doubt we're having (https://github.com/facebook/react/issues/3262). I'll let you know as soon as possible :)

hardchor commented 9 years ago

Cheers mate, really appreciate the effort. I'm following that thread.

I've tried setting state inside shouldComponentUpdate and get an error (and the state isn't being updated either), so that suggests it's not a good idea to do it there.

I can't spot any other callbacks that seem suitable for what we're trying to achieve. It's possible to manually trigger updates via forceUpdate(), but they explicitely discourage you from using it.

magalhas commented 9 years ago

Without feedback on the thread I'm not sure if it's safe, but maybe nextState can be mutated inside @shouldComponentUpdate (though I seriously doubt it)

hardchor commented 9 years ago

I've tried it out and it can. It still feels wrong to mutate state inside what is effectively a validator method.

Reading through facebook's own react-backbone TodoMVC example, they do indeed trigger forceUpdate on model change (https://github.com/tastejs/todomvc/blob/gh-pages/examples/react-backbone/js/app.jsx#L38)

magalhas commented 9 years ago

Well there's not really the need to trigger forceUpdate, also I feel that the method may get deprecated soon enough.

Lets wait for feedback on the other thread. I really think a @componentWillReceiveState hook would be awesome but I also think this won't happen.

hardchor commented 9 years ago

One last comment on this, then I'll shut up ;)

I don't know the motives behind moving models/collections to state, but the way I understand the flux flow is that there's a unidirectional flow of information.

Props (as opposed to state) are immutable from the component's point of view, so I believe they'd be more suitable for containing data coming from parent components (such as models or collections). This discourages them from being changed inside the component.

magalhas commented 9 years ago

@hardchor up until v0.8 everything was inside @props, the reality is that models and collections data is mutable so it makes all the sense to store them inside @state, though the references (passed from parent components, or not) reside inside @props because that's the way React works (props for top-to-bottom communication).

Regarding your reference to Flux, flux is a pattern and is not the only way on how to use React, anyway our mixin approach is fully "compliant" with the Flux pattern.

magalhas commented 9 years ago

Also feel free to never shut up. it took me half an year to agree on moving data from @props to @state and it was with other people feedback that I was able to "open my eyes" :+1:

magalhas commented 9 years ago

On a last note: @setProps is getting deprecated pretty soon so mutating props on the component that owns them will probably be impossible in incoming React versions.

tsjoberg commented 9 years ago

@magalhas hey just saw this library and looking to see if you can reference me to the issues/comments that discuss moving from @props to @state. Would love to see the discussion/rationale and points for either.

Thanks!

magalhas commented 9 years ago

@tsjoberg there wasn't an extensive discussion over any issue. There were some random thoughts on different issues about "why props instead of state?" where I defended props usage. One major advantage is that @setState can be called on any component, while @setProps is only available at the root component. By using @setState it allows us to use the mixin wherever we want (root, child, etc, component).

We can discuss it further if you want, just let me know :+1: