Skip to content
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

Add dangerouslySetDefaultInnerHTML #2258

Closed
wants to merge 1 commit into from
Closed

Add dangerouslySetDefaultInnerHTML #2258

wants to merge 1 commit into from

Conversation

syranide
Copy link
Contributor

dangerouslySetInnerHTML is a poor fit when trying to provide initial children for other libraries to consume, while safe as long as you don't update the value, it's not very sanitary and in some ways relies implementation details. dangerouslySetDefaultInnerHTML works just like value vs defaultValue, as you would expect.

Should probably be named dangerousDefaultInnerHTML, but I kept the current naming convention until #2257 is resolved.

@sebmarkbage
Copy link
Collaborator

Not sure this convenience is needed. getDOMNode().innerHTML works and provides an indication that you're breaking out of the abstraction. If we add this, shouldn't we also provide a way to generate default content in a safer way? Such as using ReactElements?

@syranide
Copy link
Contributor Author

@sebmarkbage getDOMNode().innerHTML is not strictly equivalent though as it is not batched with the rest of the updates, it will also be a problem when web workers lands? (unless I misunderstood your comments about web workers and DOM access)

@sophiebits
Copy link
Collaborator

Also not sure I see the value in this. dangerouslySetInnerHTML is sort of an escape hatch already and you can always continue to pass the old value if you don't want to blow away manual (or contenteditable) changes. You can also set innerHTML manually from JS, as mentioned already, which is almost the same. :)

Going to close this out but reopen if you feel strongly.

@sophiebits sophiebits closed this Feb 26, 2015
@syranide
Copy link
Contributor Author

@spicyj I don't really feel much at all about this, but it strikes me as a bit odd when we promote "easy escape hatches" as an important feature (I would argue dangerouslySetInnerHTML is an important ingredient in that) that we still leave it up to the user to cache the HTML in state or guarantee that props used in the HTML aren't mutated. When really, setting a default innerHTML value is what they should be doing.

Yeah .innerHTML works, but is also not included in server-rendering.

Anyway, don't feel strongly, it just seems a bit odd.

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