Follow-up to initial Trusted Types support #16795
Conversation
| // TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204 | ||
| const isURL = trustedTypes.isURL ? trustedTypes.isURL : value => false; | ||
| toStringOrTrustedType = value => { | ||
| if (typeof value === 'string') { |
There was a problem hiding this comment.
I guess numbers should also go here.
| return '' + value; | ||
| } | ||
| export let toStringOrTrustedType: any => string | TrustedValue = toString; | ||
| if (enableTrustedTypesIntegration && typeof trustedTypes !== 'undefined') { |
There was a problem hiding this comment.
This seems like it would make this function not inlinable if the flag is on or dynamic which isn’t great because it used to be a simple ’’ + that gets inlined.
There was a problem hiding this comment.
That’s actually what got me looking at it in the first place — it wasn’t getting inlined with a dynamic flag in FB bundle. Lemme think about this more.
There was a problem hiding this comment.
If the flag is on, it's already not being inlined, even before this PR. Probably because the body is bigger. Since we're doing a function call either way, I think a smaller function (as in this PR) would be better than a larger one. It's also a single call (in this PR) rather than two calls (as in before).
There was a problem hiding this comment.
I'm going to get this in because it's not being inlined either way with the flag on. If we want to fix this, we'll have to inline it manually at concrete callsites.
There was a problem hiding this comment.
Why do we toString in user space instead of letting the browser do it again?
Seems like we only need it in some IE legacy thing and/or for the javascript: URL check so we should only ever do this at all in those cases.
There was a problem hiding this comment.
It's for IE9 I think? We could feature-test it I guess.
| return <div dangerouslySetInnerHTML={{__html: this.state.inner}} />; | ||
| } | ||
| it('should not stringify trusted values for dangerouslySetInnerHTML', () => { | ||
| const ttObject1 = { |
There was a problem hiding this comment.
I would move this to beforeEach as it's being used in a few places.
| toStringOrTrustedType = value => { | ||
| if ( | ||
| typeof value === 'object' && | ||
| (isHTML(value) || isScript(value) || isScriptURL(value) || isURL(value)) |
There was a problem hiding this comment.
This fails with Illegal invocation error (is* functions need to be bound to the factory i.e. window.[tT]rustedTypes.
This addresses a few things I've noticed since merging #16157.
toStringOrTrustedType.toStringOrTrustedTypetotoStringdirectly, and then adds the TT checks on top when the flag is on. I've inlinedisTrustedTypesValueinto that branch since it wasn't useful by itself.isHTMLand friends early to avoid property access hit in the hot path. I'm assumingtrustedTypesmethods don't care aboutthisvalues. This might make mocking a bit more difficult but IMO this makes sense because we decide early on whether to use them or not. As opposed to TT support potentially "turning on" later in the app. This does mean, however, that TT needs to be initialized before ReactDOM to be acknowledged.toStringOrTrustedTypecall with'' + valuenow fails a particular test so we're covering them all.setInnerHTMLa bit to remove the duplicateinnerHTMLassignment which is easy to accidentally mess up. The special path now has an early return.