Repository navigation
Conversation
![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 adds support for recognizing Dart and Flutter web apps compiled with WebAssembly (Wasm) or modern web libraries (such as package:web) by checking for the presence of dart:js_interop or dart:js_interop_unsafe in addition to dart:html. It also introduces comprehensive unit tests for this detection logic. However, a critical issue was identified where serviceManager can be null (e.g., in offline apps), which would cause a null pointer exception when accessing isDartWebApp. This needs to be safely handled.
| /// Part of the modern Dart web JS interop library suite. | ||
| const dartJsInteropUnsafeLibraryUri = 'dart:js_interop_unsafe'; | ||
|
|
||
| /// Libraries whose presence indicates the connected app is a Dart web app. | ||
| /// | ||
| /// [dartHtmlLibraryUri] is kept so older apps (including google3) are still | ||
| /// detected. | ||
| const dartWebLibraryUris = [ | ||
| dartHtmlLibraryUri, | ||
| dartJsInteropLibraryUri, | ||
| dartJsInteropUnsafeLibraryUri, |
There was a problem hiding this comment.
Remove dartJsInteropUnsafeLibraryUri and its entry in dartWebLibraryUris. libraryUriAvailableNow matches with startsWith, so 'dart:js_interop' already matches dart:js_interop_unsafe. Also, dart:js_interop_unsafe imports dart:js_interop, so it is never loaded on its own. Update the CHANGELOG entry to match.
| test( | ||
| 'isDartWebApp is true when dart:js_interop_unsafe is available', | ||
| () async { | ||
| final serviceManager = _FakeServiceManager( | ||
| availableLibraries: ['dart:js_interop_unsafe'], | ||
| ); | ||
| final app = ConnectedApp(serviceManager); | ||
| expect(await app.isDartWebApp, isTrue); | ||
| expect(app.isDartWebAppNow, isTrue); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Delete this test. It passes only because the 'dart:js_interop' query prefix-matches 'dart:js_interop_unsafe', so it doesn't exercise the entry its name describes.
| group('constants', () { | ||
| test('dartWebLibraryUris contains all web library URIs', () { | ||
| expect( | ||
| dartWebLibraryUris, | ||
| containsAll([ | ||
| dartHtmlLibraryUri, | ||
| dartJsInteropLibraryUri, | ||
| dartJsInteropUnsafeLibraryUri, | ||
| ]), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Delete this group. It just restates the const list and can't catch a regression.
| @override | ||
| Future<bool> libraryUriAvailable(String uri) async { | ||
| return libraryUriAvailableNow(uri); | ||
| } |
There was a problem hiding this comment.
Drop this unless a test needs it. Nothing in this PR calls libraryUriAvailable on FakeServiceManager; the new tests use their own _FakeServiceManager.
a8a2f0c to
190f2c5
Compare
Fixes #9973
ConnectedApp.isDartWebApppreviously only checked fordart:html, so dart2wasm /package:webapps were not detected. It now also checksdart:js_interopanddart:js_interop_unsafe, short-circuiting on the first match.ConnectedApplibraryUriAvailabletoFakeServiceManager