martyjs / marty

A Javascript library for state management in React applications
http://martyjs.org
MIT License
1.09k stars 77 forks source link

Add warning about `createContainer` and `contextTypes` #327

Closed jimmed closed 9 years ago

jimmed commented 9 years ago

This issue had my colleague bogged down for a good couple of hours - I hope this warning will help prevent others falling into the same trap!

jhollingworth commented 9 years ago

Hmm, interesting one. I'm wondering if theres a way that Marty can ensure that the app contextType is there before we render?

taion commented 9 years ago

You could make the context types property on the container class be read-only.

jhollingworth commented 9 years ago

@jimmed can you give me an example of how you were setting content types?

Was it

class User extends React.Component {}

User.contextTypes = { user: ... }

or

var UserContainer = Marty.createContainer(User, {
  fetch: {
    user() { ... }
  }
})

UserContainer.contextTypes = { user: ... }

or something else?

jimmed commented 9 years ago

The latter of the two examples, so like this:

var UserContainer = Marty.createContainer(User, {
  fetch: {
    user() { ... }
  }
})

UserContainer.contextTypes = { user: ... }

The safe workaround was along these lines:

import assign from 'lodash/object/assign'; // or another Object.assign implementation...

const UserContainer = Marty.createContainer(User, { ... });

assign(UserContainer.contextTypes, { user: ... });
jhollingworth commented 9 years ago

You can pass in contextTypes into createContainer which will do the merge for you

var UserContainer = Marty.createContainer(User, {
  contextTypes: { user: ... }
  fetch: {
    user() { ... }
  }
})
jimmed commented 9 years ago

Nice! Are you happy to update the docs yourself accordingly, or should I?

(making @moretti aware of this solution)

moretti commented 9 years ago

This is great, thanks!

taion commented 9 years ago

@jhollingworth

What do you think about using Object.defineProperty to set contextTypes on the container so that this error condition isn't possible?