Supporting passing context to static rendering #429
Conversation
test/staticRender-spec.js
Outdated
|
|
||
| describeWithDOM('render', () => { | ||
| describe('context', () => { | ||
| describeIf(!REACT013, 'context', () => { |
There was a problem hiding this comment.
@hunterbmt it doesn't look like this describeIf block is wrapping all the new context tests
There was a problem hiding this comment.
That's correct @aweary. I fixed it. Sorry for this basic mistake
|
Hello there, is this still happening? I would really like this feature, thanks! |
|
@hunterbmt are you still interested in completing this PR? If not, @gerardmrk, if you can point me to a commit that adds the documentation, I can cherry-pick it onto this PR, and do the rebase. |
|
@ljharb I will finish it today (GMT +7). Sorry for this delay, I thought I finished it already but obviously, I'm not 😿 |
|
@ljharb could you help me to double check and merge this PR ? Regards, |
lencioni
dismissed
their stale review
I don't have time to review again right now and I don't want to be a blocker.
src/render.jsx
Outdated
| if (options.context && (node.type.contextTypes || options.childContextTypes)) { | ||
| const childContextTypes = node.type.contextTypes || {}; | ||
| if (options.childContextTypes) { | ||
| objectAssign(childContextTypes, options.childContextTypes); |
There was a problem hiding this comment.
Won't this mutate node.type.contextTypes if it exists?
There was a problem hiding this comment.
@hunterbmt let's avoid mutating node.type.contextTypes. We can just assign childContextTypes a newly constructed object if options.childContextTypes exists
edit: yeah do what @ljharb recommended below
Otherwise it LGTM! Great work and thanks for doing this 🎉
|
or better: const childContextTypes = objectAssign({}, node.type.contextTypes || {}, options.childContextTypes); |
|
@ljharb nice catch. I updated the PR. |
|
@ljharb could we merge this PR ? |
|
@hunterbmt sure, could you rebase onto latest master? |
|
@ljharb I already do that via github UI (you could check the last "merge" commit) |
|
@hunterbmt that's a merge, not a rebase. A rebase would leave behind zero merge commits, and requires being done on the command line. If you're unable to do it, I can take care of it for you. |
|
@ljharb I rebased master into my branch. Could you help me to merged this one ? |
|
@hunterbmt would you also mind checking the "allow edits" checkbox in the right hand column? |
|
@ljharb Already done. |
Supporting passing context to static rendering. Not yet support .context() API