Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove toString of dangerouslySetInnerHTML #17773
Conversation
As far as I can tell, the toString call was added here: caae627#diff-5574f655d491348f422bca600ff6711dR887 It was never really needed. Subsequently when we added Trusted Types, this needed to be changed to a special call but we really should just always let it pass through.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9b1d183:
|
|
I guess I added it for consistency with the call to updateTextContent: . |
sebmarkbage
merged commit edeea07
into
facebook:master
This was referenced
This was referenced
This was referenced
|
I only noticed this PR today. Thanks Sebastian! To my understanding, there's still explicit stringification left in DOMPropertyOperations, that only is removed when the TT feature flag is enabled. Is there a reason why React explicitly stringifies those values? Is it only because of IE8/9 issue where DOM APIs wouldn't stringify on their own, or is there some other reason? The ulterior motive for the questions is of course enabling TT support by default in React, but I want to understand the existing constraints. For React DOM stringification is pretty much the only blocker. |
|
Yes, I think this was because of IE9 (which we have not yet officially dropped). |
|
If this is only about the IE9 bug, perhaps we could feature test for this bug once, instead of stringifying always? That should work and would be quite simple. The only backwards compatibility issue would be for environments that monkey patch DOM APIs as React might start passing non-string values. It has bitten us in the past, but feels like a reasonable change for a major version release. |
|
What is the motivation? Is it to enable TT support sooner? If we made this change alone but did not enable the flag, what else would be missing for a full integration? |
Yes.
For client side integration the only problem is stringification of values which are later assigned to DOM sinks. This is already handled with TT support behind the feature flag. The missing part would be to handle user warnings when using script tags in components and when using dangerouslySetInnerHTML in svg element. |
|
I think we're open to a PR doing feature testing if you can verify it actually works on IE9. (You'd also need a few polyfills for testing the fix: Map/Set/rAF.) |
To clarify, removing the stringification would make majority of the apps TT-ready (i.e. applications wishing to enforce TT would add an appropriate header and start producing TT values when interpolating into sensitive sinks). It's quite likely this single change could be added without backwards compatibility issues - that's what we're trying to find out.
Only a warning in
All are perfectly valid - and these issues are not blocking for Trusted Type adopters, whereas stringification is blocking (not terribly, but some patterns require workarounds e.g. one can't interpolate into |
As far as I can tell, the toString call was added here:
caae627#diff-5574f655d491348f422bca600ff6711dR887
However, we don't toString the HTML in the initial creation. Only updates. It was never really needed.
Subsequently when we added Trusted Types, this needed to be changed to a special call but we really should just always let it pass through.
Interestingly we should probably always let values pass through except for
when we need to do something special with the string which is rare.