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

Add a server endpoint for serving devtools extensions #6094

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

kenzieschmoll
Copy link
Member

This PR

  • moves extension_model.dart to packages/devtools_shared so that we can reuse this model from the devtools server
  • creates ExtensionManager that manages moving devtools extension assets to build/devtools_extensions so that the server can serve them (implemented in https://dart-review.googlesource.com/c/sdk/+/305043)
  • adds a server endpoint to serve devtools extensions

work towards #1632

@kenzieschmoll kenzieschmoll requested a review from a team as a code owner July 25, 2023 18:10
Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

First pass.

@@ -0,0 +1,74 @@
// Copyright 2023 The Chromium Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming, this is a straight move from package:devtools_app/src/extensions/embedded/extension_model.dart right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added dart doc to the members and some validation in the factory constructor so it should be re-reviewed. I also made the json parsing for the icon code point able to handle both strings and ints

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

Another pass.

final extensions = <Extension>[];
for (final extension in extensions) {
final config = extension.config;
if (config is! Map) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If config is always expected to be Map, why don't we restrict it to Map<Object, Object?> in Extension?

Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion at dart-lang/tools#129 (comment)

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

LGTM with one last suggestion.

codePoint = codePointFromJson as int? ?? defaultCodePoint;
}

final name = json[nameKey] as String?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use pattern matching here for JSON validation:

  if (json
      case {
        nameKey: String name,
        pathKey: String path,
        issueTrackerKey: String issueTracker,
        versionKey: String version,
      }) {
    return DevToolsExtensionConfig._(
      name: name,
      path: path,
      issueTrackerLink: issueTrackerLink,
      version: version,
      materialIconCodePoint: codePoint,
    );
  }
  throw StateError(
    'missing required fields ${nullFields.toString()} in the extension '
    'config.json',
  );

Copy link
Member Author

Choose a reason for hiding this comment

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

I will address this comment as part of #6097.

@kenzieschmoll kenzieschmoll merged commit 5d062a7 into flutter:master Jul 26, 2023
21 of 23 checks passed
@kenzieschmoll kenzieschmoll deleted the dte-server branch July 26, 2023 16:42
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