Skip to content

Conversation

elliette
Copy link
Contributor

No description provided.

@@ -153,7 +153,7 @@ class ExtensionDebugger implements RemoteDebugger {

@override
void close() => _closed ??= () {
_closeController.add(WipEvent({}));
_closeController.add({});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The content of this event does not matter, its existence is used to notify clients that the connection should be closed. Using a WipEvent caused a type error, since WipEvent expects non-null parameters.

@elliette elliette marked this pull request as ready for review August 1, 2022 21:09
@elliette elliette requested a review from annagrin August 1, 2022 21:09
@@ -189,9 +187,9 @@ class MockLoadStrategy implements LoadStrategy {

@override
shelf.Handler get handler =>
(request) => (request.url.path == 'someDummyPath')
(request) async => (request.url.path == 'someDummyPath')
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like MockLoadStrategy is very similar to the FakeStrategy in fakes.dart - should we unify this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done!

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! Left a few comments.

? shelf.Response.ok('some dummy response')
: null;
: shelf.Response.notFound('not found');
Copy link
Contributor

Choose a reason for hiding this comment

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

moduleForServerPath returns a Future<String?> in the base interface - should we make it the same here, to flush any compilation errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks!

@@ -164,7 +162,7 @@ void main() {

tearDownAll(() async {
DartUri.clear();
await outputDir?.delete(recursive: true);
await outputDir.delete(recursive: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this matters but this will prevent retries if the creation of outputDir fails, but previously retries would be technically possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving for now, we can update if needed

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! The remaining comment is optional - we might leave the outputDir as optional just to be safe or wait for the scenario I described in there to happen and fix it then.

@elliette elliette merged commit b1a2742 into dart-lang:master Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants