Skip to content

modifying DWDS Injector to always inject client and introduce useDwdsWebSocketConnection flag #2629

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jyameo
Copy link
Contributor

@jyameo jyameo commented Jun 2, 2025

  • Updated DWDS to always inject the client and use useDwdsWebSocketConnection flag to control to control communication protocol (web-socket vs chrome).
  • Temporarily trigger main from the client based on the flag. This will be used by the ddcLibraryBundle/Web-server code path for now and will be repaced when the socket-based DWDS is ready.

Related to dart-lang/sdk#60289.

Sibling PR in flutter tools: flutter/flutter#170612.

@jyameo jyameo requested a review from srujzs June 2, 2025 19:00
@jyameo jyameo requested a review from nshahan June 2, 2025 19:00
@jyameo
Copy link
Contributor Author

jyameo commented Jun 2, 2025

There are some test failures with test/frontend_server_callstack_test.dart: shared context | callStack | expression evaluation succeeds on parent frame (failed).

The test seems to be passing when I run it locally so I'm not sure why it's failing in the CI. Any ideas? @bkonyi @srujzs

@nshahan
Copy link
Contributor

nshahan commented Jun 2, 2025

I have a fix out for the existing test failures that should clear them up. Feel free to land your change before that makes it's way in though.
https://dart-review.googlesource.com/c/sdk/+/431951

@jyameo jyameo requested a review from srujzs June 3, 2025 19:04
@jyameo jyameo changed the title modifying DWDS Injector to always inject client and introduce runMainAtStart flag modifying DWDS Injector to always inject client and introduce useDwdsWebSocketConnection flag Jun 13, 2025
Copy link
Contributor

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

Looks generally good, thanks Jessy! I haven't gone through your spreadsheet to make sure things are aligned there, but that'd be a good exercise to do.

await _extensionUri,
);
}
// Always inject the debugging client and hoist the main function.
Copy link
Contributor

Choose a reason for hiding this comment

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

This means we're injecting the client when doing AMD + web-server, which wasn't the case before unless start-paused was passed, is that right? I don't have an objection to that addition, but I just want to make sure that doesn't break anything and this is intentional.

),
if (!(dartModuleStrategy == 'ddc-library-bundle')) {
if (_isChromium) {
_sendConnectRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this in a method that gets used here and below.

..instanceId = dartAppInstanceId
..entrypointPath = dartEntrypointPath,
),
if (!(dartModuleStrategy == 'ddc-library-bundle')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

or simply != :D

..instanceId = dartAppInstanceId
..entrypointPath = dartEntrypointPath,
),
if (!(dartModuleStrategy == 'ddc-library-bundle')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How should we handle useDwdsWebSocketConnection here? It seems like if that flag was passed then we should run main immediately as well in the AMD format, right?

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.

3 participants