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

Remove toString of dangerouslySetInnerHTML #17773

Merged
merged 1 commit into from Jan 4, 2020

Conversation

sebmarkbage
Copy link
Collaborator

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.

@sizebot
Copy link

sizebot commented Jan 4, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 9b1d183

@sizebot
Copy link

sizebot commented Jan 4, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 9b1d183

As far as I can tell, the toString call was added here:

facebook@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.
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 4, 2020

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:

Sandbox Source
gallant-butterfly-t2e2l Configuration

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I added it for consistency with the call to updateTextContent:

this.updateTextContent('' + nextContent);
.

@sebmarkbage sebmarkbage merged commit edeea07 into facebook:master Jan 4, 2020
@koto
Copy link
Contributor

koto commented Aug 10, 2020

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.

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

Yes, I think this was because of IE9 (which we have not yet officially dropped).
We plan to enable the TT flag in 18.

@koto
Copy link
Contributor

koto commented Aug 10, 2020

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.

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

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?

@Siegrift
Copy link
Contributor

What is the motivation? Is it to enable TT support sooner?

Yes.

If we made this change alone but did not enable the flag, what else would be missing for a full integration?

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.

@gaearon
Copy link
Collaborator

gaearon commented Aug 10, 2020

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.)

@koto
Copy link
Contributor

koto commented Aug 10, 2020

If we made this change alone but did not enable the flag, what else would be missing for a full integration?

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.

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.

The missing part would be to handle user warnings when using script tags in components and when using dangerouslySetInnerHTML in svg element.

Only a warning in __DEV__ mode is gated on the flag there, the behavior stays the same. I don't think this ever needs to change. Ultimately there's several ways of how to approach these:

  • supporting these patterns and not warning devs at all,
  • documenting as a known issue, and describing workarounds (in React, or elsewhere)
  • warning gated on a flag (current behavior)
  • removing the support for these patterns (with deprecation warnings?)

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 <iframe srcdoc>).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants