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

Using of fetch is ignored #54

Closed
LKay opened this issue Jan 26, 2015 · 10 comments
Closed

Using of fetch is ignored #54

LKay opened this issue Jan 26, 2015 · 10 comments
Labels

Comments

@LKay
Copy link

LKay commented Jan 26, 2015

I have a browserify/node kind of application and wanted to use this module to bind backbone collections/models to react components. Unfortunately when I call fetch() method on collection/model then component not listening for changes on sync events. The factory approach does not work in that case as I need a component body. I try to set props such as collections using setDefaultProps. Here is my example file:

"use strict";

var React         = require("react"),
    Router        = require("react-router"),
    $             = require("jquery"),
    _             = require("underscore"),
    Backbone      = require("backbone"),
    backboneMixin = require("backbone-react-component");

Backbone.$ = $;

var FieldModel = Backbone.Model.extend({
    idAttribute: "name"
});

var FieldsCollection = Backbone.Collection.extend({
    model: FieldModel,
    url: "/fake-api/fields.json",

    parse: function (response) {
        return response.fields;
    }
});

var Fields = React.createClass({
    mixins: [backboneMixin],

    getDefaultProps: function () {
        return {
            collection: {
                fields: new FieldsCollection()
            }
        };
    },

    componentDidMount: function () {
        this.getCollection().fields.fetch();
    },

    createEmptyRow: function () {
        console.warn("empty row");
        return "";
    },

    createFieldRow: function (field) {
        console.warn(field);
        return "";
    },

    createTable: function () {
        console.warn(this.props.fields);
        return (
            <div className="table-responsive">
                <table className="table table-striped table-bordered">
                    <thead>
                        <th>Row #</th>
                    </thead>
                    <tbody>
                        {this.props.fields.length === 0 ? this.createEmptyRow() : this.props.fields.map(this.createFieldRow)}
                    </tbody>
                </table>
            </div>
        );
    },

    render: function () {
        return (
            <div>
                {this.createTable()}
            </div>
        );
    }
});

module.exports = Fields;
@LKay LKay changed the title Using of fetch is ignored Using of fetch is ignored Jan 26, 2015
@magalhas
Copy link
Owner

I'm pretty sure the problem is because you're setting the collection inside @getDefaultProps, can you please just try and set the collection outside @getDefaultProps to validate if that's the problem? Something like <Fields collection={{fields: new FieldsCollection()}} />

@LKay
Copy link
Author

LKay commented Jan 27, 2015

In this case the component is being passed to another processor so I can't use factory pattern and initialise compiled component. For me it is obviously bug as it shouldn't be any difference when setting models/collections on the compiled component or within component body, should it?

EDIT: The component is passed directly to the router (react-router) as a route handler.

@magalhas
Copy link
Owner

Keep in mind that getDefaultProps gets cached. I need to investigate further to see if there's no problem on fallbacking models/collections to the ones returned by getDefaultProps.

On Terça-feira, 27/01/2015 at 01:44, Karol Janyst notifications@github.com, wrote:
Message contents can't be shown right now

@LKay
Copy link
Author

LKay commented Jan 27, 2015

Ok, I debugged the code a little bit and it is, indeed possible to set the props in the getDefaultProps at they are passed to the wrapper object. The problem, I suppose lies here

    ...
    // Start listeners if this is a root node and if there's DOM
    if (!component._owner && typeof document !== 'undefined') {
      this.startModelListeners();
      this.startCollectionListeners();
    }
    ...

as the mixin expects the component to be root one, which is not always possible. If it's not the root component then the listeners are not binded. What I did for workaround for now is to change state of the collection manually but it should be possible to set binding for non-root components as well.

        this.getCollection().fields.fetch({
            // FIXME: temporary workaround
            success: function (collection) {
                self.setState({
                    fields: collection.toJSON()
                });
            }
        });

Btw. If it's not the root components the factory approach does not work as well. (<Field collection={new Collection()} />)

@magalhas
Copy link
Owner

Yes using setState for non root components is a solution that I've been
thinking of implementing.

Not sure though if using setProps when there's a root component with the
mixin and setState when it does not is a good rule of thumb.

Btw, are you using v0.8 of the mixin?

On Tuesday, January 27, 2015, Karol Janyst notifications@github.com wrote:

Ok, I debugged the code a little bit and it is, indeed possible to set the
props in the getDefaultProps at they are passed to the wrapper object.
The problem, I suppose lies here

...
// Start listeners if this is a root node and if there's DOM
if (!component._owner && typeof document !== 'undefined') {
  this.startModelListeners();
  this.startCollectionListeners();
}
...

as the mixin expects the component to be root one, which is not always
possible. If it's not the root component then the listeners are not binded.
What I did for workaround for now is to change state of the collection
manually but it should be possible to set binding for non-root components
as well.

    this.getCollection().fields.fetch({
        // FIXME: temporary workaround
        success: function (collection) {
            self.setState({
                fields: collection.toJSON()
            });
        }
    });


Reply to this email directly or view it on GitHub
#54 (comment)
.

Cumprimentos,
José Magalhães

@LKay
Copy link
Author

LKay commented Jan 27, 2015

I'm using 0.7.3 now. About mixing setProps and setState is not a good reactish approach so it should be consistent. I think the wrapper should should use only states as they are dynamic and changing within components scope. Props should be only used to set up the initial state for models/collections within component body. Sharing the models/collections between components and should be taken care by either global referential variables or Flux approach.

Maybe I could made some contribution and post PR later this week.

@magalhas
Copy link
Owner

Feel free to tackle it down. If you do please do it over the master, there are already some changes by using contexts, feature that will be included in the incoming version of React.

@magalhas
Copy link
Owner

magalhas commented Feb 5, 2015

@LKay I'm about to start working on this, Considering what you've mentioned earlier, sharing models/collections between parent and child components can be done cleanly through React contexts, it's already working like that on the master branch. Anyway, the major changes I'm considering for the next release:

  • Internally @setState will be always used instead of @setProps, so you're model/collections data will be available under @state
  • @getModel and @getCollection no longer crawls the dirty ´_ownerreference (which will be removed on incoming React release), instead they are able to grab the model and collection instances through@context` (new React feature)

I think after the next release the API will get fully stable and we could consider versioning 1.0.0, though I think we should wait for React v1.0.0 release for that to happen (and who knows when that will be).

@magalhas
Copy link
Owner

@LKay master already include changes listed in #55 which probably fix your use case.

@magalhas
Copy link
Owner

v0.8.0-beta.1 is released and it fixes this

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

No branches or pull requests

2 participants