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

Revert "🤖 Merge PR #60691 [react] Allow TrustedHTML in `dangerously… #60837

Merged

Conversation

peterblazejewicz
Copy link
Member

…SetInnerHTML` by @eps1lon"

This reverts commit 9c58088.

We cannot force people to use skip lib check option to avoid introduced problem (trusted-types are custom types, not library type from lib.dom, so default approach with overrides/redeclaration does not work and was missed on review)

Reerts #60691

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

…in `dangerouslySetInnerHTML` by @eps1lon"

This reverts commit 9c58088.

Reerts DefinitelyTyped#60691
@typescript-bot
Copy link
Contributor

typescript-bot commented Jun 16, 2022

@peterblazejewicz Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

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

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 60837,
  "author": "peterblazejewicz",
  "headCommitOid": "f358396cc7a27c427e61185f9e57e9e8c83882e0",
  "mergeBaseOid": "5e9837afeea9dd75b9f69febd076e81d851977b4",
  "lastPushDate": "2022-06-16T17:02:28.000Z",
  "lastActivityDate": "2022-06-16T20:13:40.000Z",
  "mergeOfferDate": "2022-06-16T20:05:08.000Z",
  "mergeRequestDate": "2022-06-16T20:13:40.000Z",
  "mergeRequestUser": "peterblazejewicz",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/global.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/test/index.ts",
          "kind": "test"
        },
        {
          "path": "types/react/v16/global.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v16/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v16/test/index.ts",
          "kind": "test"
        },
        {
          "path": "types/react/v17/global.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v17/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v17/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": "approved",
      "reviewer": "jakebailey",
      "date": "2022-06-16T20:04:25.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1157922375,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 16, 2022

trusted-types should change here since we've been using the same approach we've been using with other DOM types.

With this precedent any library can prevent shipping features in @types/react. Or we need an integration test suite that lists libraries that we need to ensure compat with.

@peterblazejewicz
Copy link
Member Author

peterblazejewicz commented Jun 16, 2022

Or we need an integration test suite that lists libraries that we need to ensure compat with.

one can add import 'trusted-types' to React types tests (only to tests), that would fail with the change from #60691

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 16, 2022

But why this library?

Next step will be me adding every existing DOM interface to react/index.d.ts so that libraries will not be able to publish incorrect types. The approach of lib.dom.d.ts has been working for years. Why is trusted-types allowed to break that?

@peterblazejewicz
Copy link
Member Author

I really do not know, it's optional for use as dependency, until formalized. DOMPurify wors that way, it adds this explicitely:

/// <reference types="trusted-types"/>

There is no enough momentu to make that api part of lib.dom I assume:
microsoft/TypeScript#30024

I'd like to just avoid situation people are forced to use skip lib check, I understand what you're folks doing with @types/react

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 16, 2022

I'd like to just avoid situation people are forced to use skip lib check,

Have you tried reaching out to trusted-types and ask why they can't use the same approach as lib.dom.d.ts? How do they envision working once TrustedHTML gets added to lib.dom.d.ts?

@peterblazejewicz
Copy link
Member Author

peterblazejewicz commented Jun 16, 2022

Sorry, I will not reach anybody, I understand a normal way is to revert a change that introduced unexpected outcomes.
There is pending PR to move trusted api into lib.dom:
microsoft/TypeScript-DOM-lib-generator#1246
and it covers a topic from this change IMO:
microsoft/TypeScript-DOM-lib-generator#1246 (comment)

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 16, 2022

With the same rationale we won't be able to add TrustedHTML to lib.dom.d.ts because trusted-types breaks. This is really unorthodox behavior but I guess there's some MSFT internal communication I'm missing.

@DangerBotOSS
Copy link

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 f358396

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Jun 16, 2022
@peterblazejewicz
Copy link
Member Author

Sorry, this is not subject of this PR (which is revert a change), sorry. I can honestly say I've made mistake with review of #60691,
Let someone review, thx!

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

I personally don't know much about trusted types, but it does seem like #60691 has broken things, so I think the safest thing thing to do right now is to revert it and try again, just to restore the status quo.

The way trusted-types does seem incongruent with the way lib.dom.d.ts is defined, so it would seem like its definitions should be change, but again, I am no trusted types expert.

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Jun 16, 2022
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Jun 16, 2022
@peterblazejewicz
Copy link
Member Author

Ready to merge 💘

@typescript-bot typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Jun 16, 2022
@typescript-bot typescript-bot merged commit d1c6213 into DefinitelyTyped:master Jun 16, 2022
@peterblazejewicz peterblazejewicz deleted the revert/60691 branch June 16, 2022 20:14
@typescript-bot typescript-bot removed this from Recently Merged in New Pull Request Status Board Jun 25, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented Jul 9, 2022

The way trusted-types does seem incongruent with the way lib.dom.d.ts is defined, so it would seem like its definitions should be change, but again, I am no trusted types expert.

Proposed #61160 so that we can reland #60691

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants