Skip to content

Fix for listening to custom streams in DWDS. - #2011

Merged
elliette merged 3 commits into
masterfrom
kInvParams
Mar 7, 2023
Merged

elliette merged 3 commits into
masterfrom
kInvParams

Conversation

@CoderDake

Copy link
Copy Markdown

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
Copy Markdown
Author

CC: @DanTup @bkonyi @elliette

@DanTup

DanTup commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

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

elliette commented Mar 2, 2023

Copy link
Copy Markdown
Contributor

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

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

bkonyi commented Mar 3, 2023

Copy link
Copy Markdown
Collaborator

LGTM pending Elliott's approval


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

Copy link
Copy Markdown
Contributor

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)

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.

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

4 participants