-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Added context support #62
Conversation
|
||
#### Common Gotchas | ||
|
||
- `.setContext()` can only be used on a wrapper that was initially created with a call to `mount()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this guaranteed, ie, will it throw an exception? I don't see any tests that cover this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. will add some tests.
a089c9f
to
c26c6bc
Compare
@ljharb tests added. comments addressed. |
c26c6bc
to
9acb9d1
Compare
Changes LGTM - @goatslacker should weigh in on the context stuff tho since he's got more experience with it than I |
Looking at the coverage info with the line numbers mismatched is a PITA. I'm going to submit a followup PR that fixes istanbul line numbers, as well as improve coverage where needed |
// specified in both the options AND the child component defines `contextTypes` statically. | ||
// In that case, we define both a `getChildContext()` function and a `childContextTypes` prop. | ||
Object.assign(spec, { | ||
childContextTypes: node.type.contextTypes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're mutating spec here. Is that ok? Seems like a bad idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's ok.
LGTM |
Added context support
this.component = renderIntoDocument( | ||
<ReactWrapperComponent | ||
Component={nodes.type} | ||
props={nodes.props} | ||
context={options.context} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not due to this diff, but the airbnb styleguide shows this closing wrapper to be back 2 spaces. https://github.com/airbnb/javascript/tree/master/react#alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tru dat
Super useful PR. May I ask what is the reason for |
to: @ljharb @goatslacker
Adds support for context with
mount
andshallow
.The API looks like:
Additionally, there is a
.setContext(newContext)
prototype method on each wrapper class.Fixes #49
Note: this will constitute a minor change (new feature, backwards compatible)