[react] Support anything with toString() (such as TrustedHTML) as input for dangerouslySetInnerHTML.

[lgarron]

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.
• Provide a URL to documentation or source code which provides context for the suggested changes: https://developer.mozilla.org/en-US/docs/Web/API/TrustedHTML

This change allows passing TrustedHTML to dangerouslySetInnerHTML in React. React correctly passes this to innerHTML in a way that is compatible with CSP enforcement of the Trusted Types API. We are prototyping a use case for this inside the GitHub codebase.

Since this is a nested property on an interface, I couldn't find a reasonable way to augment the react namespace to support this inside a single project. So I thought I'd send this PR to see if we could consider marking the functionality in @types/react directly.

[typescript-bot]

@lgarron Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• react — on npm, on unpkg

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Most recent commit is approved by a DT maintainer

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 26 days — it is still unreviewed!

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 60417,
  "author": "lgarron",
  "headCommitOid": "40151cd5c393ec6547475ef4557ef32950be3bda",
  "mergeBaseOid": "b6a234377c5edea999fc23777189aad8cdf5d29f",
  "lastPushDate": "2022-05-18T22:03:01.000Z",
  "lastActivityDate": "2022-06-06T18:29:23.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/test/index.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "johnnyreilly",
        "bbenezech",
        "pzavolinsky",
        "ericanderson",
        "DovydasNavickas",
        "theruther4d",
        "guilhermehubner",
        "ferdaber",
        "jrakotoharisoa",
        "pascaloliv",
        "hotell",
        "franklixuefei",
        "Jessidhia",
        "saranshkataria",
        "lukyth",
        "eps1lon",
        "zieka",
        "dancerphil",
        "dimitropoulos",
        "disjukr",
        "vhfmag",
        "hellatan",
        "priyanshurav"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "eps1lon",
      "date": "2022-05-18T20:38:08.000Z",
      "abbrOid": "1fc726a"
    }
  ],
  "mainBotCommentID": 1130507965,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[eps1lon]

I think we should rather use the same approach we do with the dom related stuff: use an empty interface for TrustedHTML and let users take care of the proper type. We shouldn't use the polyfill directly here.

For more guidance check how we test the other DOM interfaces.

[lgarron]

Ah, thanks, I thought trusted-types were the global types to be used. microsoft/TypeScript-DOM-lib-generator#1246 is stalled, so they're not available globally yet. I'll try to take a look.

[typescript-bot]

@lgarron One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

[eps1lon]

Also: Could you check in which version support for trusted types was added to React?

[eps1lon]

I don't think React has actual support for trusted types yet.

I think this only works incidentally now because TrustedHTML implements toString().

So instead of accepting only string we should probably just widen the accepted types to { toString(): string } i.e. there's no need to mention TrustedHTML in the typings at all.

[lgarron]

I don't think React has actual support for trusted types yet.

I think this only works incidentally now because TrustedHTML implements toString().

So instead of accepting only string we should probably just widen the accepted types to { toString(): string } i.e. there's no need to mention TrustedHTML in the typings at all.

From what I can tell, React passes the input to innerHTML without converting to a string. I think accepting anything with toString() makes sense, though, because that string value does get used in the end. (That is, {toString: () => "<b>hello world</b>"} can be passed both to innerHTML and dangerouslySetInnerHTML.)

[eps1lon]

From what I can tell, React passed the input to innerHTML without converting to a string.

Looks like it: https://codesandbox.io/s/trustedhtml-kfxbhz

I think accepting anything with toString() makes sense, though, because that string value does get used.

Nah, if it really is just assigned we should simply accept the same type as HTMLElement.prototype.innerHTML. But then we'd still need to add TrustedHTML to https://github.com/microsoft/TypeScript-DOM-lib-generator/blob/bdf9e19b9676d8b1a4ea4fde7407354004e7104a/baselines/dom.generated.d.ts#L8907

[lgarron]

Nah, if it really is just assigned we should simply accept the same type as HTMLElement.prototype.innerHTML. But then we'd still need to add TrustedHTML to https://github.com/microsoft/TypeScript-DOM-lib-generator/blob/bdf9e19b9676d8b1a4ea4fde7407354004e7104a/baselines/dom.generated.d.ts#L8907

That certainly makes sense. If it makes sense to wait on that, would you have any advice for any TypeScript projects that want to pass TrustedHTML to dangerouslySetInnerHTML today, without suppressing errors?

[typescript-bot]

@lgarron The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

[DangerBotOSS]

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

react (unpkg)

was missing the following properties:

1. unstable_act

Generated by 🚫 dangerJS against 40151cd

[typescript-bot]

@eps1lon Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[eps1lon]

would you have any advice for any TypeScript projects that want to pass TrustedHTML to dangerouslySetInnerHTML today, without suppressing errors?

Unfortunately module augmentation doesn't work for existing properties so all you can do is error suppression.

[lgarron]

would you have any advice for any TypeScript projects that want to pass TrustedHTML to dangerouslySetInnerHTML today, without suppressing errors?

Unfortunately module augmentation doesn't work for existing properties so all you can do is error suppression.

Do I understand correctly that the best way forward is to advocate for microsoft/TypeScript-DOM-lib-generator#1246 to land, and to change dangerouslySetInnerHTML to take the type of the Element.innerHTML setter from the DOM types?

[lgarron]

change dangerouslySetInnerHTML to take the type of the Element.innerHTML setter from the DOM types?

Is there a good way to do this? It looks like types from lib.dom.ts are not available inside the React definitions right now — would it be worth importing those vs. adding a toString() alternative directly to the type?

[eps1lon]

Do I understand correctly that the best way forward is to advocate for microsoft/TypeScript-DOM-lib-generator#1246 to land, and to change dangerouslySetInnerHTML to take the type of the Element.innerHTML setter from the DOM types?

Yep

Is there a good way to do this?

I don't know. One difficulty is how this change will integrate into react-native

[typescript-bot]

Re-ping @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @Hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @eps1lon, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @hellatan, @priyanshurav:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

[typescript-bot]

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @lgarron.

(Ping @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @Hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @eps1lon, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @hellatan, @priyanshurav.)

[eps1lon]

Alternate proposal #60691