Adding support for Build Daemon + Frontend Server with expression eval. - #2836
Adding support for Build Daemon + Frontend Server with expression eval.#2836Markzipan wants to merge 90 commits into
Conversation
b86e719 to
79b7fa2
Compare
nshahan
left a comment
nshahan
left a comment
There was a problem hiding this comment.
I know you are doing some manual testing, but are those at a point that you can link some CLs/PRs so we can track? I want to avoid landing this too early if we still have work to do before rolling into the Dart SDK, Flutter, and google3.
| /// A reader for Dart sources and related source maps provided by the Frontend | ||
| /// Server. | ||
| class FrontendServerAssetReader implements AssetReader { | ||
| class FrontendServerAssetReader extends AssetReader { |
There was a problem hiding this comment.
Is the implements -> extends change needed? Looks like there is no implementation to inherit in AssetReader?
|
|
||
| /// A reader for resources provided by a proxy server. | ||
| class ProxyServerAssetReader implements AssetReader { | ||
| class ProxyServerAssetReader extends AssetReader { |
| return null; | ||
| } | ||
|
|
||
| @override |
| import 'package:shelf/shelf.dart' as shelf; | ||
|
|
||
| class TestAssetServer implements AssetReader { | ||
| class TestAssetServer extends AssetReader { |
| } | ||
|
|
||
| class FakeAssetReader implements AssetReader { | ||
| class FakeAssetReader extends AssetReader { |
srujzs
left a comment
srujzs
left a comment
There was a problem hiding this comment.
Most of my comments are centered around some of the classes we're adding here.
| final PackageConfig packageConfig; | ||
| abstract class PathResolver { | ||
| /// Computes the server path for a given application URL. | ||
| String? appUriToServerPath(String appUrl, {bool? useDebuggerModuleNames}); |
There was a problem hiding this comment.
nits: Might be worth detailing when we should expect nulls as returns here and below.
| class PackageUriMapper { | ||
| final _logger = Logger('PackageUriMapper'); | ||
| final PackageConfig packageConfig; | ||
| abstract class PathResolver { |
There was a problem hiding this comment.
Since we actually enumerate the derived classes ourselves, can we consider making these and the subclasses final or some other class modifier? Or do we expect other PathResolvers?
| bool get useDebuggerModuleNames; | ||
| } | ||
|
|
||
| abstract class PathResolverBase implements PathResolver { |
There was a problem hiding this comment.
I might have missed another implementation but consider flatting the hierarchy here so PathResolverBase and PathResolver are one class.
| /// build daemon. | ||
| /// | ||
| /// We assume the build daemon already has a Frontend Server intialized. | ||
| class DaemonExpressionCompiler implements ExpressionCompiler { |
There was a problem hiding this comment.
Also consider using a class modifier here to seal the hierarchy.
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| /// Encapsulates asset file naming conventions and schemes. | ||
| abstract class AssetScheme { |
There was a problem hiding this comment.
Consider class modifiers here and derived classes as well.
| Future<JSArray<JSObject>> hotRestartBegin(String? reloadedSourcesPath) async { | ||
| assert( |
There was a problem hiding this comment.
I'm totally fine handling this in here, but I'm guessing we're handling it in here to check that this path exists for hot restart without breaking the AMD and DDC module formats?
| // BSD-style license that can be found in the LICENSE file. | ||
|
|
||
| /// Encapsulates asset file naming conventions and schemes. | ||
| abstract class AssetScheme { |
There was a problem hiding this comment.
Can AssetScheme live as a getter within PathResolver? Mostly wondering if that's something a user needs to configure different from PathResolver.
3 participants
Changes at a glance
DWDS
asset_scheme.dart: AddedAssetSchemeto handle platform-specific file extensions (e.g.,.ddc.jsvs.dart.lib.js,.ddc.dillvs.dart.lib.dill) withBuildRunnerAssetSchemeandFrontendServerAssetSchemedefault impls.asset_reader.dart: RemovedPackageUriMapperin favor ofPathResolverinterface with default impls:FrontendServerPathResolver,BuildRunnerPathResolver, andFlutterPathResolver.web_path_translator.dartto hold asset translation rules (lib/segments, asset-scheme-specific renaming, etc.).loader.dart: AddedMetadataLoader(with default implMergedMetadataLoader) to allow platform-specific reading/parsing of metadata files.strategy.dart:LoadStrategynow requires anAssetSchemeand accepts aMetadataLoader.BuildSettingsnow acceptsuseDebuggerModuleNames.dwds.dart: Now exports:AssetScheme,MetadataLoader, and related impls.frontend_server_strategy_provider.dart: AddedFrontendServerBuildDaemonStrategyProviderto support Frontend Server + Build Daemon config with hot reload.daemon_expression_compiler.dart: AddedDaemonExpressionCompilerto forward expression evaluation requests directly to the instantiated Frontend Server worker via socket.location.dart: Updated sourcemap parsing to account for FES-specific URIs.Webdev and Serving
dev_workflow.dart: Spawns a backgroundbuild_frontend_server:fes_managersubprocess whenwebHotReloadis enabled. This creates a persistent shared Frontend Server (with a persistent scratch space) that can receive expression eval, hot reload, and hot restart requests while communicating with the build daemon.webdev_server.dart: UsesFrontendServerBuildDaemonStrategyProviderwhenwebHotReloadis enabled. Writesfes_manager_configto signal to build runner that a shared FES should be used. Now passesPathResolverandBuildSettings.app_domain.dart: Now supports hot reload requests (instead of rejecting them).server_manager.dart: PassesBuildDaemonClienttoWebDevServerfor build results to be accessible.Tests
context.dart: Test fixtures now specify aPathResolver. Rewrote_createBuildRunnerDdcLibraryBundleAssetHandlerto serve compiled outputs directly from the build cache or scratch space..ddc_merged_metadatavia thefes_manager.BuildStatus.succeededmessage early to avoid blocking downstream completers and locking.reloaded_sources.json. This is unlike a real app but works well for the test fixtures.build_web_compilersto4.8.7in test fixtures.