Repository navigation
Emit instance *Trusted overloads on *TrustedTypesExtension - #607
Conversation
Move non-static `*Trusted` setters and methods onto companion `*TrustedTypesExtension` extensions so DOM `extension type`s expose a single instance member per JS property name for `@JSExport()` and `package:mockito`. - Name generated extensions `*TrustedTypesExtension` to avoid colliding with `DocumentExtension` and `ElementExtension` in `lib/src/helpers/extensions.dart`. - Keep `Worker.trusted`, `SharedWorker.trusted`, and static `Document.parseHTMLUnsafeTrusted` on their `extension type`s. - Add a structural `api_test` check verifying DOM extension types have no colliding JS instance member names.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the JS interop generator to emit Trusted Types helper methods and setters inside dedicated extension classes (e.g., *TrustedTypesExtension) rather than directly on the extension types themselves, preventing potential JS member name collisions. It also adds a test to verify that no colliding JS instance member names exist across public extension types. Feedback on the new test suggests adding a safety check in _effectiveJsName to ensure a setter's name actually ends with '=' before truncating it, preventing potential RangeError exceptions.
Is there also work happening on figuring out how to make this landable in google3, or handling the roll with same-cl fixes? |
|
Yes — once |
There was a problem hiding this comment.
This is fine, but iiuc, the central problem is that there are packages in google3 that rely on the JSAny-typed member, right? We can migrate the non-third_party ones, but for the ones that are rolled in, we're in a bind because this necessitates either:
- Publishing
package:webv2 asap so the rolled-in packages can use it, and then we roll them all together into google3. - Adding a patch into google3 for each package until we're ready for (1).
- Modifying the rolled-in packages in google3 directly (bad, because a new roll of those packages might reintroduce the problem).
|
I'm digging in to mockito to see if IT should be fixed here. We can land this now if it's helpful and revisit before we publish. I kind of like the trusted bits being next to the normal bits just for readability. |
I'm not sure there's a recourse here without extension members because you have two Dart definitions for one JS member. You can imagine unioning the two member types, but the issue of choosing one member definition still exists. |
|
@srujzs we HAVE a pattern for maintaining v1x and v2 – it's just a bit of a chore. We don't really have any fixes in pkg:web that are critical for Google3, right? We could just...wait. This seems like something we should chat about. |
Waiting SGTM. I guess we might want helpers here to then be rolled into google3 and that might complicate things. |
|
@srujzs we can also move the "new" work into a long-lived branch and just do an (ugly) revert commit if we want to keep history clean and move in smaller changes. |
Move non-static
*Trustedsetters and methods onto companion*TrustedTypesExtensionextensions so DOMextension types keep a single instance member per JS property name for@JSExport()andpackage:mockito, addressing review feedback on #596.*TrustedTypesExtensionto avoid colliding withDocumentExtensionandElementExtensioninlib/src/helpers/extensions.dart.Worker.trusted,SharedWorker.trusted, and staticDocument.parseHTMLUnsafeTrustedon theirextension types because@JSExport()only exports instance members and Dart extensions cannot declare constructors.