Skip to content

Support URL tunnelling (pass dwds UrlEncoder through to editors via daemon)#44271

Merged
DanTup merged 14 commits intoflutter:masterfrom
DanTup:tunelled-url-proposal
Dec 12, 2019
Merged

Support URL tunnelling (pass dwds UrlEncoder through to editors via daemon)#44271
DanTup merged 14 commits intoflutter:masterfrom
DanTup:tunelled-url-proposal

Conversation

@DanTup
Copy link
Copy Markdown
Contributor

@DanTup DanTup commented Nov 6, 2019

@jonahwilliams @devoncarew this is a possible idea for supporting debugging on remote web environments.

The problem that this is trying to solve is that when we launch a web app through Flutter and want to use the Dart Debug extension, the app gets some injected JavaScript that includes the URL for the DWDS debug proxy service (which the Dart Debug extension will read and connect to). In a remote environment, the URL that is injected here is something like http://localhost:8123, but when this code executes on the end users machine, that URL is not accessible.

In order to make it accessible, we need to pass the URL back to the client (eg. VSO/GitPod) and ask it to set up a tunnel and translate the URL to one that will work for the end user. In the case of DWDS, this needs to be passed back in to Flutter so that Flutter can provide a handler to DWDS to map the URL as it's injecting the client.

My suggestion is to allow the Flutter daemon to send a request to the client (this is a request/response in the opposite direction to usual) to do this. The request would only be sent when a URL needs to be mapped (which generally means the client is specifically passing --no-web-browser-launch for web or --start-paused to the web-server device).

What do you think?

@DanTup DanTup added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 6, 2019
@jonahwilliams
Copy link
Copy Markdown
Contributor

Seems similar to how the tool already sets up port-forwarding for connected devices, right? At least the client configuration part would need to be exposed in dwds I think though

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Nov 6, 2019

Seems similar to how the tool already sets up port-forwarding for connected devices, right?

The idea is similar, but for this the tunnelling/forwarding can't be done by Flutter, it needs to be exposed to the client to do (consider VS Online - the tunnelling involves setting up a proxy/tunnel into the VSO infrastructure, so we need to delegate it to them - which is done with an API call inside the VS Code extension).

At least the client configuration part would need to be exposed in dwds I think though

My understanding is that dart-lang/webdev#804 gives us the hook we need for dwds to call us back when it needs to translate the URL, so from there we can forward the request on to the client.

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Nov 20, 2019

@jonahwilliams @devoncarew I've pushed the implementation I'm currently using here for testing. It allows a client editor to pass a flag that enables this functionality, which will call back to the editor (using a request that goes in the opposite direction) when a URL needs mapping (this request is just passed through Flutter from dwds).

This is how URLs used within dwds (for example the URL of the debug service that the debug extension will use) are exposed by the infrastructure and the proxied URL passed back to dwds.

May be easier to review with whitespace ignored -> https://github.com/flutter/flutter/pull/44271/files?utf8=%E2%9C%93&diff=unified&w=1

(this will need tests, but I want to check this seems like a reasonable direction!)

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Nov 25, 2019

The build failure seems to be that I put the typedef for UrlTunneller in web_fs.dart and that's not allowed to be imported in run.dart. Is there a more appropriate place for this? @jonahwilliams

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.

You can't expose anything from a build_runner folder through the tool normally, it needs to go through an indirection of some kind. The issue is that the build_runner deps don't exist in google3 or fuchsia-source, so there is a risk of the tool not building

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only thing being imported is a simple typedef. I need it so I can inject one into context (see https://github.com/flutter/flutter/blob/3ee2f16793fab9a96768ab5f2c69032fbfb387ee/packages/flutter_tools/lib/src/commands/run.dart#L383-L385).

Is there somewhere suitable I can move that typedef that would allow me to remove this? (I guess it could pretty much be anywhere, I just couldn't find anything similar to be consistent with.

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.

It doesn't matter what you import, because it will force all of the transitive imports to be resolved which will fail once it hits a build_runner/daemon import.

I would place it somewhere in base (net.dart?)

@DanTup DanTup marked this pull request as ready for review November 28, 2019 13:56
@DanTup DanTup changed the title Proposal for supporting URL tunnelling Support URL tunnelling (pass dwds UrlEncoder through to editors via daemon) Dec 3, 2019
@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Dec 3, 2019

@jonahwilliams @devoncarew Still looking for some feedback/review on this :-)

@jonahwilliams jonahwilliams self-requested a review December 3, 2019 15:07
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.

I'd like to avoid creating nested DI scopes if possible. Could we plumb this value directly through to the daemon instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I pushed be95c6399b9c854000036f5cc374b7bacda2f628 - is this the sort of thing you had in mind? The condition is now handled in daemon.dart (it passes the urlTunneller function depending on the options).

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.

Looks good to me, thank you!

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.

For now, no - though we'll need to do something here in the future

@jonahwilliams
Copy link
Copy Markdown
Contributor

The changes modulo the daemon API seem good to me. For that I would want @devoncarew to review

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Dec 3, 2019

Great, thanks! This still needs tests, but I'll wait to hear if @devoncarew is ok with the daemon API before I put tests around it.

Comment thread packages/flutter_tools/doc/daemon.md Outdated
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.

@DanTup @jonahwilliams - the spec change here lgtm

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Dec 4, 2019

I pushed some tweaks:

  • the spec said params for the response instead of result (result is what the daemon uses for it's outbound responses, and what JSON-RPC dictates)
  • the implementation expects the result to be the string url directly, not an object with a field as-spec'd (I kept the spec and fixed the implementation, since having an object would allow us to extend this in future if we need more than just a url back)
  • added a test that the calling exposeUrl does send the JSON request and handles the response

Let me know if this all still looks good.

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Dec 10, 2019

Rebased as there was a conflict (it was just a new import on the same line as one I added).

@jonahwilliams please lmk if you're happy with the tweaks mentioned in the comment above :-)

Copy link
Copy Markdown
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@DanTup DanTup merged commit 4944622 into flutter:master Dec 12, 2019
@DanTup DanTup deleted the tunelled-url-proposal branch December 12, 2019 18:44
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants