mongodb-js / connect-backbone-to-react

Connect Backbone Models and Collections to React.
https://www.npmjs.com/package/connect-backbone-to-react
Apache License 2.0
27 stars 5 forks source link

Alternative API #1

Closed hswolff closed 7 years ago

hswolff commented 7 years ago

This is a discussion about what the API for this function should look like. The goal here is to maximize the efficiency of using this function.

To lay out the problem, right now we have a total of 4 arguments that are used:

And right now those 4 arguments are being used like this:

const ConnectedComponent = connectBackboneToReact(modelsMap, modelsToProps, options)(WrappedComponent);

The thought here being that you can predefine how models are being mapped to properties which you could easily add to any WrappedComponent.

So you could

const connectCreator = connectBackboneToReact(modelsMap, modelsToProps, options);
const ConnectedComponent = connectCreator(WrappedComponent)
const ConnectedComponent2 = connectCreator(WrappedComponent2)

@jrbalsano suggested an alternative API that I think is pretty good and possibly should replace the current one.

He suggests to change the argument syntax around to allow for easier exporting. Something close to:

const ConnectedComponent = connectBackboneToReact(modelsToProps, options)(WrappedComponent);

ReactDOM.render(
  <ConnectedComponent {...modelsMap} />.
  domEl
);

I'm kind of inclined with the 2nd way at this point as it allows for a user to define the props that a component will get, and it will work nicely alongside react-redux.

// file: MyComponent.js
class MyComponent extends Comonent {
    render() {
        return (
            <div>Hello Moto</div>
        );
    }
}

module.exports = MyComponent;

module.exports.MyComponentBackbone = connectBackboneToReact(
    modelsToProps,
    options
)(MyComponent);

module.exports.MyComponentRedux = connect(mapStateToProps)(MyComponent);

The current API doesn't really allow for that type of export signature.

Thoughts on changing to new API? Any alternative suggestions?

This is the only blocking IMO for pushing this to 1.0.0.

hswolff commented 7 years ago

I think the general difference in approach from current method to the proposed method is that:

jjgonecrypto commented 7 years ago

I'm inclined towards the second approach. I like passing in the modelMap as props and like that it's closer to the react-redux approach already established

hswolff commented 7 years ago

So one limiting aspect of the proposed method that I just thought of is it doesn't permit arbitrary properties to be given to the wrapped component, it would just be able to accept properties that would otherwise make up the modelMaps objects.

i.e.:

<ConnectedComponent {...modelsMap} />
// or
<ConnectedComponent user={userModel} allUsers={userCollection}  />

If we wanted to add other arbitrary properties that control visual display such as

<ConnectedComponent {...modelsMap} showLabels={false} />

I'm not quite sure how to do that.

Perhaps we have one required property on the ConnectedComponent?

<ConnectedComponent data={modelsMap} showLabels={false} />

And that's where we get our modelsMap from, allowing other arbitrary properties to be given and passed down as well.

jjgonecrypto commented 7 years ago

Good point. I think that data compromise works. @jrbalsano what about you?

hswolff commented 7 years ago

Closed in favor of #2