-
-
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hunterbmt it doesn't look like this describeIf
block is wrapping all the new context tests
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.
That's correct @aweary. I fixed it. Sorry for this basic mistake
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.
Thanks for your contribution! Will you please add documentation for this change and rebase onto the latest master when you have a chance?
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, |
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.
Seems good but let's get more eyes on it.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this mutate node.type.contextTypes
if it exists?
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 catch, let's not mutate
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.
@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