Skip to content

Fix for listening to custom streams in DWDS.#2011

Merged
elliette merged 3 commits intomasterfrom
kInvParams
Mar 7, 2023
Merged

Fix for listening to custom streams in DWDS.#2011
elliette merged 3 commits intomasterfrom
kInvParams

Conversation

@CoderDake
Copy link

@CoderDake CoderDake commented Mar 2, 2023

Fixes Dart-Code/Dart-Code#4414 (comment)

When listening to a stream, DDS does a streamListen on the VMService. If the VMService, doesn't recognize the stream it will respond with kInvalidParams. This lets DDS know that the stream is nonstandard, so it can handle the events for that stream on it's own.

This PR changes DWDS to behave like the VMService in this regard. Fixing this will allow us to streamListen on custom streams using DWDS.

@CoderDake
Copy link
Author

CoderDake commented Mar 2, 2023

CC: @DanTup @bkonyi @elliette

@DanTup
Copy link
Contributor

DanTup commented Mar 2, 2023

Thanks! This change LGTM (it's what I tested locally and definitely worked), but you may want the opinion of someone more familiar with DWDS than I 🙂

@elliette
Copy link
Contributor

elliette commented Mar 2, 2023

Thanks! Could you add a test case? Here are where we have test cases for the other stream event types:

webdev/dwds/test/chrome_proxy_service_test.dart

Lines 1973 to 1998 in 85f35d4

test('Timeline', () async {
expect(service.streamListen('Timeline'), completion(_isSuccess));
});
test('Stdout', () async {
expect(service.streamListen('Stdout'), completion(_isSuccess));
expect(
service.onEvent('Stdout'),
emitsThrough(predicate((Event event) =>
event.kind == EventKind.kWriteEvent &&
String.fromCharCodes(base64.decode(event.bytes!))
.contains('hello'))));
await context.tabConnection.runtime.evaluate('console.log("hello");');
});
test('Stderr', () async {
expect(service.streamListen('Stderr'), completion(_isSuccess));
final stderrStream = service.onEvent('Stderr');
expect(
stderrStream,
emitsThrough(predicate((Event event) =>
event.kind == EventKind.kWriteEvent &&
String.fromCharCodes(base64.decode(event.bytes!))
.contains('Error'))));
await context.tabConnection.runtime.evaluate('console.error("Error");');
});

@bkonyi
Copy link
Collaborator

bkonyi commented Mar 3, 2023

LGTM pending Elliott's approval

elliette
elliette reviewed Mar 3, 2023
View reviewed changes
dwds/test/chrome_proxy_service_test.dart Outdated

test('custom stream', () async {
expect(
() => service.streamListen('VM'),
Copy link
Contributor

@elliette elliette Mar 3, 2023

Choose a reason for hiding this comment

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

We handle the VM stream (which is why the test is failing)

@elliette elliette merged commit c37d419 into master Mar 7, 2023
@elliette elliette deleted the kInvParams branch March 7, 2023 20:37
@elliette elliette mentioned this pull request Mar 7, 2023
Merged
@DanTup DanTup mentioned this pull request Mar 8, 2023
Closed
@DanTup DanTup mentioned this pull request Apr 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@elliette elliette elliette approved these changes

Assignees

No one assigned

Labels

package:dwds

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SDK DAPs fail to connect to web apps on latest code because subscribing to the ToolEvent stream fails

4 participants

@CoderDake @DanTup @elliette @bkonyi