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

Scrap Marty.Component in favour of Marty.createContainer #206

Merged
merged 11 commits into from Mar 22, 2015
Merged

Conversation

jhollingworth
Copy link
Contributor

FB just blogged about how Relay works. This makes a lot of sense and gets around a load of the issues with Marty.Component (e.g. contextTypes, having to inherit from a base class). This PR gets rid of Marty.Component and replicates the Relay.createContainer API.

module.exports = Marty.createContainer(User, {
  listenTo: [UserStore, FriendsStore],
  fetch: {
    user() {
      return UserStore.getUser(this.props.id)
    },
    friends() {
      return FriendsStore.getFriends(this.props.id)
    }
  },
  // optional
  done(fetches, component) {
    return <User {...fetches} {...component.props} />
  },
  // when any of the fetches have failed
  failed(error, component) {
    return <Error />;
  },
  // when any of the fetches are pending
  pending(component) {
    return <User user={{}} {...component.props}/>;
  }
});

To do

  • Rename container to registry
  • Create Marty.createContainer
    • failed
    • done
    • pending
  • Get rid of Marty.Component
  • Update all examples of using Marty.Component

Resolves #204

@jhollingworth jhollingworth changed the title Edit New issue Scrap Marty.Component in favour of Marty.createContainer Scrap Marty.Component in favour of Marty.createContainer Mar 20, 2015
@jhollingworth
Copy link
Contributor Author

@dariocravero
Copy link
Contributor

👍 came here to suggest just that and spotted this PR.

Totally on for dropping Marty.Component in favour of this approach, it makes lots of sense to get away from a custom Component class :).
Also, good call on renaming Container to Registry and having done, failed and pending at the container level, I'd imagine that the object comes clean to the Component without any when stuff?

EDIT: Missed that ES feature :/, don't mind the example question. Quick one, looking at the source, shouldn't the example read like this?

It might also be a good idea to keep Relay semantics in place? So instead of calling it fetch it could be named queries? I know that Marty queries kind of do a different thing but still, maybe it's out of question but perhaps Marty queries could be renamed instead?

@pbomb
Copy link
Contributor

pbomb commented Mar 22, 2015

We have some container components (view-controllers) in Marty v0.8 where some of the fetches depend on the results other ones. I like the convenience of this proposed API, but it doesn't allow for fetches to depend on others or for possibly more complicated behavior. How about only putting your implementation of getState on the container (which puts each fetch on the state) if there's not already a getState function defined on the config object? Then I can continue to use an implementation for getState like I already do for the v0.8 StateMixins for these situations.

@jhollingworth
Copy link
Contributor Author

@dariocravero awesome, glad you like it 😄 I'd like to keep the fetch keyword for now as I'd like to leave the door open for integrating with relay in the future

@jhollingworth
Copy link
Contributor Author

@pbomb How about if I allowed fetch to either be a function or an object literal?

module.exports = Marty.createContainer(User, {
  fetch() {
    return {
      user: UserStore.getUser(this.props.id),
      friends: FriendsStore.getFriends(this.props.id)
    }
  }
});

@pbomb
Copy link
Contributor

pbomb commented Mar 22, 2015

Yeah, I thought about that too. I like it!

On Sun, Mar 22, 2015, 9:53 AM James Hollingworth notifications@github.com
wrote:

@pbomb https://github.com/pbomb How about if I allowed fetch to either
be a function or an object literal containing functions?

module.exports = Marty.createContainer(User, {
fetch() {
return {
user: UserStore.getUser(this.props.id),
friends: FriendsStore.getFriends(this.props.id)
}
}
});


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

Conflicts:
	dist/node/lib/context.js
	dist/node/lib/create.js
	dist/node/lib/storeObserver.js
	dist/node/lib/utils/resolve.js
	lib/context.js
	lib/create.js
@jhollingworth
Copy link
Contributor Author

cool, implemented in 4ce9143

jhollingworth added a commit that referenced this pull request Mar 22, 2015
Scrap Marty.Component in favour of Marty.createContainer
@jhollingworth jhollingworth merged commit 7f029bb into v0.9 Mar 22, 2015
@jhollingworth jhollingworth deleted the container branch March 22, 2015 17:49
@dariocravero
Copy link
Contributor

Class ;)

@pbomb
Copy link
Contributor

pbomb commented Mar 23, 2015

It would be great to be able to specify the display name is the container component, either by padding or in our by adding 'Container' to the display name of the inner component. We like to record metrics and these names are helpful.

@jhollingworth
Copy link
Contributor Author

@pbomb would "{InnerComponentDisplayName}Container" be fine? e.g.

<FooContainer>
   <Foo />
</FooContainer>

@pbomb
Copy link
Contributor

pbomb commented Mar 23, 2015

I think so

On Mon, Mar 23, 2015, 6:16 AM James Hollingworth notifications@github.com
wrote:

@pbomb https://github.com/pbomb would
"{InnerComponentDisplayName}Container" be fine? e.g. FooContainer


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

@jhollingworth
Copy link
Contributor Author

@pbomb 718dafb

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.

None yet

3 participants