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

Refactor: Update the parameters for DWDS.start #2231

Merged
merged 13 commits into from
Sep 20, 2023

Conversation

elliette
Copy link
Contributor

@elliette elliette commented Sep 13, 2023

This PR:

  • refactors the parameters for Dwds.start so that all of the miscellaneous strings / boolean parameters are collected into either DebugSettings or AppMetadata. This is to make it easier to add optional parameters to the Dwds.start method, since we won't need to update the flutter_tools DwdsLauncher each time: https://github.com/flutter/flutter/blob/367203b3011fc1752cfa1f51adf9751d090c94e6/packages/flutter_tools/lib/src/isolated/devfs_web.dart#L46
  • sets DebugSettings and AppMetadata as globals so that they can be accessed from anywhere (instead of having to pipe some of their fields down to the Injector as we were previously doing)
  • renames the parameter names to consistently use devTools/DevTools instead of devtools/Devtools
  • makes the parameter enableDebugging is no longer required but optional. If not set, it defaults to true

This is preparatory work for #2198, because we will be adding a new parameter to Dwds.start (the workspace name)

Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

Thanks Elliott, really appreciate cleaning dwds.start up! I think we have and issue for this work somewhere, will try to find it an update. Left a few comments.

dwds/lib/dart_web_debug_service.dart Outdated Show resolved Hide resolved
dwds/lib/src/config/debug_settings.dart Outdated Show resolved Hide resolved
dwds/lib/src/utilities/globals.dart Outdated Show resolved Hide resolved
dwds/test/dart_uri_test.dart Outdated Show resolved Hide resolved
dwds/lib/src/config/app_metadata.dart Outdated Show resolved Hide resolved
dwds/lib/src/handlers/injector.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

Thanks, left some more comments

dwds/lib/dart_web_debug_service.dart Outdated Show resolved Hide resolved
dwds/lib/src/config/app_metadata.dart Outdated Show resolved Hide resolved
dwds/lib/src/handlers/injector.dart Show resolved Hide resolved
set globalLoadStrategy(LoadStrategy strategy) => _globalLoadStrategy = strategy;
LoadStrategy get globalLoadStrategy => _globalLoadStrategy;
class ToolConfiguration {
LoadStrategy loadStrategy;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can all those fields be final, or late final if needed? I don't think we want to allow changing them during the run of dwds?

dwds/test/fixtures/server.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/utilities.dart Outdated Show resolved Hide resolved
webdev/lib/src/serve/webdev_server.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

LGTM but would lie a follow up PR where we have pre-defined test configurations and/or helpers to create them. Consider passing tool configuration to contest constructor as well.

@elliette elliette merged commit d2dae56 into dart-lang:master Sep 20, 2023
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants