Add option to not use module name in MetadataProvider (#2529)

DDC library bundle format does not provide module names as
part of the ModuleMetadata.

- To support this discrepancy, a flag is added to MetadataProvider
to either use the module name or the module path to identify the
module.
- createProvider is added so LoadStrategys can create
MetadataProviders based on whether the module format uses the
module name.
- Cleans up some getters and methods in LoadStrategy implementations
so that getters are grouped together.

Author: srujzs

dwds/CHANGELOG.md

@@ -5,7 +5,11 @@
 - Replace deprecated JS code `this.__proto__` with `Object.getPrototypeOf(this)`. - [#2500](https://github.com/dart-lang/webdev/pull/2500)
 - Migrate injected client code to `package:web`. - [#2491](https://github.com/dart-lang/webdev/pull/2491)
 - Deprecated MetadataProvider's, CompilerOptions', SdkConfiguration's & SdkLayout's soundNullSafety. - [#2427](https://github.com/dart-lang/webdev/issues/2427)
-- Add load strategy and an unimplemented hot restart strategy for DDC library bundle format.
+- Add load strategy and an unimplemented hot restart strategy for DDC library
+  bundle format.
+- Added `useModuleName` option to `MetadataProvider` to determine whether or not
+  to use the provided `name` in a `ModuleMetadata`. Metadata provided by DDC
+  when using the library bundle format does not provide a useful bundle name.
 
 ## 24.1.0
 

dwds/lib/src/debugging/metadata/module_metadata.dart

@@ -101,6 +101,8 @@ class ModuleMetadata {
   ///
   /// Used as a name of the js module created by the compiler and
   /// as key to store and load modules in the debugger and the browser
+  // TODO(srujzs): Remove once https://github.com/dart-lang/sdk/issues/59618 is
+  // resolved.
   final String name;
 
   /// Name of the function enclosing the module

dwds/lib/src/debugging/metadata/provider.dart

@@ -22,6 +22,8 @@ class MetadataProvider {
   final Map<String, String> _moduleToModulePath = {};
   final Map<String, List<String>> _scripts = {};
   final _metadataMemoizer = AsyncMemoizer();
+  // Whether to use the `name` provided in the module metadata.
+  final bool _useModuleName;
 
   /// Implicitly imported libraries in any DDC component.
   ///
@@ -64,7 +66,11 @@ class MetadataProvider {
         'dart:ui',
       ];
 
-  MetadataProvider(this.entrypoint, this._assetReader);
+  MetadataProvider(
+    this.entrypoint,
+    this._assetReader, {
+    required bool useModuleName,
+  }) : _useModuleName = useModuleName;
 
   /// A sound null safety mode for the whole app.
   ///
@@ -113,6 +119,8 @@ class MetadataProvider {
   ///   web/main
   /// }
   ///
+  /// If [_useModuleName] is false, the values will be the module paths instead
+  /// of the name except for 'dart_sdk'.
   Future<Map<String, String>> get scriptToModule async {
     await _initialize();
     return _scriptToModule;
@@ -127,13 +135,14 @@ class MetadataProvider {
   ///   web/main.ddc.js.map
   /// }
   ///
-  ///
+  /// If [_useModuleName] is false, the keys will be the module paths instead of
+  /// the name.
   Future<Map<String, String>> get moduleToSourceMap async {
     await _initialize();
     return _moduleToSourceMap;
   }
 
-  /// A map of module path to module name
+  /// A map of module path to module name.
   ///
   /// Example:
   ///
@@ -142,12 +151,14 @@ class MetadataProvider {
   ///   web/main
   /// }
   ///
+  /// If [_useModuleName] is false, the values will be the module paths instead
+  /// of the name, making this an identity map.
   Future<Map<String, String>> get modulePathToModule async {
     await _initialize();
     return _modulePathToModule;
   }
 
-  /// A map of module to module path
+  /// A map of module to module path.
   ///
   /// Example:
   ///
@@ -156,12 +167,14 @@ class MetadataProvider {
   ///   web/main.ddc.js :
   /// }
   ///
+  /// If [_useModuleName] is false, the keys will be the module paths instead of
+  /// the name, making this an identity map.
   Future<Map<String, String>> get moduleToModulePath async {
     await _initialize();
     return _moduleToModulePath;
   }
 
-  /// A list of module ids
+  /// A list of module ids.
   ///
   /// Example:
   ///
@@ -170,6 +183,8 @@ class MetadataProvider {
   ///   web/foo/bar
   /// ]
   ///
+  /// If [_useModuleName] is false, this will be the set of module paths
+  /// instead.
   Future<List<String>> get modules async {
     await _initialize();
     return _moduleToModulePath.keys.toList();
@@ -196,8 +211,9 @@ class MetadataProvider {
               final metadata =
                   ModuleMetadata.fromJson(moduleJson as Map<String, dynamic>);
               _addMetadata(metadata);
-              _logger
-                  .fine('Loaded debug metadata for module: ${metadata.name}');
+              final moduleName =
+                  _useModuleName ? metadata.name : metadata.moduleUri;
+              _logger.fine('Loaded debug metadata for module: $moduleName');
             } catch (e) {
               _logger.warning('Failed to read metadata: $e');
               rethrow;
@@ -211,10 +227,14 @@ class MetadataProvider {
   void _addMetadata(ModuleMetadata metadata) {
     final modulePath = stripLeadingSlashes(metadata.moduleUri);
     final sourceMapPath = stripLeadingSlashes(metadata.sourceMapUri);
+    // DDC library bundle module format does not provide names for library
+    // bundles, and therefore we use the URI instead to represent a library
+    // bundle.
+    final moduleName = _useModuleName ? metadata.name : modulePath;
 
-    _moduleToSourceMap[metadata.name] = sourceMapPath;
-    _modulePathToModule[modulePath] = metadata.name;
-    _moduleToModulePath[metadata.name] = modulePath;
+    _moduleToSourceMap[moduleName] = sourceMapPath;
+    _modulePathToModule[modulePath] = moduleName;
+    _moduleToModulePath[moduleName] = modulePath;
 
     for (final library in metadata.libraries.values) {
       if (library.importUri.startsWith('file:/')) {
@@ -223,12 +243,12 @@ class MetadataProvider {
       _libraries.add(library.importUri);
       _scripts[library.importUri] = [];
 
-      _scriptToModule[library.importUri] = metadata.name;
+      _scriptToModule[library.importUri] = moduleName;
       for (final path in library.partUris) {
         // Parts in metadata are relative to the library Uri directory.
         final partPath = p.url.join(p.dirname(library.importUri), path);
         _scripts[library.importUri]!.add(partPath);
-        _scriptToModule[partPath] = metadata.name;
+        _scriptToModule[partPath] = moduleName;
       }
     }
   }
@@ -239,6 +259,9 @@ class MetadataProvider {
     for (final lib in sdkLibraries) {
       _libraries.add(lib);
       _scripts[lib] = [];
+      // TODO(srujzs): It feels weird that we add this mapping to only this map
+      // and not any of the other module maps. We should maybe handle this
+      // differently.
       _scriptToModule[lib] = moduleName;
     }
   }

dwds/lib/src/loaders/ddc.dart

@@ -163,6 +163,9 @@ class DdcStrategy extends LoadStrategy {
   @override
   String get loadModuleSnippet => 'dart_library.import';
 
+  @override
+  BuildSettings get buildSettings => _buildSettings;
+
   @override
   Future<String> bootstrapFor(String entrypoint) async =>
       await _ddcLoaderSetup(entrypoint);
@@ -208,9 +211,6 @@ window.\$dartLoader.loader.nextAttempt();
   @override
   String? serverPathForAppUri(String appUri) => _serverPathForAppUri(appUri);
 
-  @override
-  BuildSettings get buildSettings => _buildSettings;
-
   @override
   String? g3RelativePath(String absolutePath) => _g3RelativePath(absolutePath);
 }

dwds/lib/src/loaders/ddc_library_bundle.dart

@@ -140,6 +140,9 @@ class DdcLibraryBundleStrategy extends LoadStrategy {
       "function() { throw new Error('LoadStrategy.loadModuleSnippet is used. "
       "This is currently unsupported in the DDC library bundle format.'); }";
 
+  @override
+  BuildSettings get buildSettings => _buildSettings;
+
   @override
   Future<String> bootstrapFor(String entrypoint) async =>
       await _ddcLoaderSetup(entrypoint);
@@ -186,8 +189,11 @@ window.\$dartLoader.loader.nextAttempt();
   String? serverPathForAppUri(String appUri) => _serverPathForAppUri(appUri);
 
   @override
-  BuildSettings get buildSettings => _buildSettings;
+  String? g3RelativePath(String absolutePath) => _g3RelativePath(absolutePath);
 
   @override
-  String? g3RelativePath(String absolutePath) => _g3RelativePath(absolutePath);
+  MetadataProvider createProvider(String entrypoint, AssetReader reader) =>
+      // DDC library bundle format does not provide module names in the module
+      // metadata.
+      MetadataProvider(entrypoint, reader, useModuleName: false);
 }

dwds/lib/src/loaders/strategy.dart

@@ -53,16 +53,37 @@ abstract class LoadStrategy {
   /// App build settings, such as entry point, build flags, app kind etc.
   BuildSettings get buildSettings;
 
-  /// Returns the bootstrap required for this [LoadStrategy].
-  ///
-  /// The bootstrap is appended to the end of the entry point module.
-  Future<String> bootstrapFor(String entrypoint);
-
   /// A handler for strategy specific requests.
   ///
   /// Used as a part of the injected_handler middleware.
   Handler get handler;
 
+  /// Returns a loader to read the content of the package configuration.
+  ///
+  /// The package configuration URIs will be resolved relative to
+  /// [packageConfigPath], but the loader can read the config from a different
+  /// location.
+  ///
+  /// If null, the default loader will read from [packageConfigPath].
+  Future<Uint8List?> Function(Uri uri)? get packageConfigLoader => null;
+
+  /// The absolute path to the app's package configuration.
+  String get packageConfigPath {
+    return _packageConfigPath ?? _defaultPackageConfigPath;
+  }
+
+  /// The default package config path if none is provided.
+  String get _defaultPackageConfigPath => p.join(
+        DartUri.currentDirectory,
+        '.dart_tool',
+        'package_config.json',
+      );
+
+  /// Returns the bootstrap required for this [LoadStrategy].
+  ///
+  /// The bootstrap is appended to the end of the entry point module.
+  Future<String> bootstrapFor(String entrypoint);
+
   /// JS code snippet for loading the injected client script.
   String loadClientSnippet(String clientScript);
 
@@ -113,27 +134,6 @@ abstract class LoadStrategy {
   /// Returns `null` if not a google3 app.
   String? g3RelativePath(String absolutePath);
 
-  /// Returns a loader to read the content of the package configuration.
-  ///
-  /// The package configuration URIs will be resolved relative to
-  /// [packageConfigPath], but the loader can read the config from a different
-  /// location.
-  ///
-  /// If null, the default loader will read from [packageConfigPath].
-  Future<Uint8List?> Function(Uri uri)? get packageConfigLoader => null;
-
-  /// The absolute path to the app's package configuration.
-  String get packageConfigPath {
-    return _packageConfigPath ?? _defaultPackageConfigPath;
-  }
-
-  /// The default package config path if none is provided.
-  String get _defaultPackageConfigPath => p.join(
-        DartUri.currentDirectory,
-        '.dart_tool',
-        'package_config.json',
-      );
-
   /// Returns the [MetadataProvider] for the application located at the provided
   /// [entrypoint].
   MetadataProvider metadataProviderFor(String entrypoint) {
@@ -144,10 +144,15 @@ abstract class LoadStrategy {
     }
   }
 
+  /// Creates and returns a [MetadataProvider] with the given [entrypoint] and
+  /// [reader].
+  MetadataProvider createProvider(String entrypoint, AssetReader reader) =>
+      MetadataProvider(entrypoint, reader, useModuleName: true);
+
   /// Initializes a [MetadataProvider] for the application located at the
   /// provided [entrypoint].
   Future<void> trackEntrypoint(String entrypoint) {
-    final metadataProvider = MetadataProvider(entrypoint, _assetReader);
+    final metadataProvider = createProvider(entrypoint, _assetReader);
     _providers[metadataProvider.entrypoint] = metadataProvider;
     // Returns a Future so that the asynchronous g3-implementation can override
     // this method:

dwds/test/fixtures/fakes.dart

@@ -363,10 +363,10 @@ class FakeStrategy extends LoadStrategy {
   String get loadModuleSnippet => '';
 
   @override
-  String? g3RelativePath(String absolutePath) => null;
+  ReloadConfiguration get reloadConfiguration => ReloadConfiguration.none;
 
   @override
-  ReloadConfiguration get reloadConfiguration => ReloadConfiguration.none;
+  String? g3RelativePath(String absolutePath) => null;
 
   @override
   String loadClientSnippet(String clientScript) => 'dummy-load-client-snippet';
@@ -394,7 +394,7 @@ class FakeStrategy extends LoadStrategy {
 
   @override
   MetadataProvider metadataProviderFor(String entrypoint) =>
-      MetadataProvider(entrypoint, FakeAssetReader());
+      createProvider(entrypoint, FakeAssetReader());
 
   @override
   Future<Map<String, ModuleInfo>> moduleInfoForEntrypoint(String entrypoint) =>