Skip to content

Removed context paremeter to renderToString/renderToStaticMarkup. #2565

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

Merged
merged 1 commit into from
Nov 19, 2014

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Nov 19, 2014

Fixed whitespace after 'if' statement as per style guidelines

@sophiebits
Copy link
Collaborator

Thanks.

@sophiebits
Copy link
Collaborator

(Though #2566 has this and more.)

@sebmarkbage
Copy link
Collaborator

Delete this argument. The second argument shouldn't exist. It's always empty context.

@jimfb
Copy link
Contributor Author

jimfb commented Nov 19, 2014

Ok, second argument deleted.

@sebmarkbage
Copy link
Collaborator

Can you update the title of the PR for reference?

To clarify the rationale here... We shouldn't accept any contexts from the outside of top-level because we may want to connect a context from another React subtree that generated this subtree. E.g. for layers. But we may also want to put DOM specific context on there by default. E.g. current focus/selection.

@jimfb jimfb changed the title Fixed whitespace after 'if' statement as per style guidelines Removed context paremeter to renderToString/renderToStaticMarkup. Nov 19, 2014
@jimfb
Copy link
Contributor Author

jimfb commented Nov 19, 2014

I actually don't understand that answer. Wanting to "connect a context from another React subtree" or "want to put a DOM specific context on there by default" both seem like reasons we SHOULD have the second argument. Perhaps you can explain it to me in more detail on Friday.

jimfb added a commit that referenced this pull request Nov 19, 2014
Removed context paremeter to renderToString/renderToStaticMarkup.
@jimfb jimfb merged commit 6ec9529 into facebook:master Nov 19, 2014
@jimfb jimfb mentioned this pull request Nov 19, 2014
3 tasks
@jimfb jimfb deleted the fixwhitespace branch November 19, 2014 23:52
@sebmarkbage
Copy link
Collaborator

In short, it's because we don't KNOW how we will do this and as soon as we add a new API we will have 10-30 users of it by tomorrow.

We might want to auto-attach it implicitly and not require it to be passed through. Explicit pass-through doesn't really work when the context is masked.

@jimfb
Copy link
Contributor Author

jimfb commented Nov 20, 2014

Ah, got it! Makes sense :).

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.

3 participants