Skip to content
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

Deprecate DTDConnectionInfo and replace with a class that can handle separate private/exposed URIs #8291

Merged
merged 7 commits into from
Sep 9, 2024

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Sep 3, 2024

This replaces DTDConnectionInfo with DTDInfo (unsure if a better name?) and marks the former as deprecated.

The new class has two URIs:

  • localUri which is the URI used for backend services on the same machine as the DTD process (this is usually a localhost URI)
  • exposedUri which is the URI that can be used from the frontend where the user is (this may or may not be the same as localUri depending on whether they're in a remote/web IDE session)

The DevTools web API returns the exposed URI (since it will be used by frontend tools) and other places that used the URI for connecting directly use the local one.

On its own, this change doesn't do anything, the intention is to roll this into the SDK and update the DevTools server code in dartdev/DDS to accept an additional optional URI which will then provide these back to the server code here (for the web API handler and extension search code).

This is work towards fixing some of the issues in Dart-Code/Dart-Code#5158.

…separate private/exposed URIs

This replaces `DTDConnectionInfo` with `DTDInfo` (unsure if a better name?) and marks the former as deprecated.

The new class has two URIs:

- `localUri` which is the URI used for backend services on the same machine as the DTD process (this is usually a `localhost` URI)
- `exposedUri` which is the URI that can be used from the frontend where the user is (this may or may not be the same as localUri depending on whether they're in a remote/web IDE session)

The DevTools web API returns the exposed URI (since it will be used by frontend tools) and other places that used the URI for connecting directly use the local one.

On its own, this change doesn't do anything, the intention is to roll this into the SDK and update the DevTools server code in dartdev/DDS to accept an additional optional URI which will then provide these back to the server code here (for the web API handler and extension search code).

This is work towards fixing some of the issues in Dart-Code/Dart-Code#5158.
@DanTup DanTup marked this pull request as ready for review September 3, 2024 16:41
@DanTup
Copy link
Contributor Author

DanTup commented Sep 3, 2024

@kenzieschmoll is this something that would be included in the release notes? (it doesn't really affect the DevTools app - at least not without other fixes - only devtools_shared users).

While I left the old typedef and made it deprecated, these changes are still breaking for any code that calls any of these methods (such as the handlers) that have had their signatures changed (this means that rolling the new devtools_shared into the SDK will also require some code changes, which I will start preparing a CL for).

@DanTup
Copy link
Contributor Author

DanTup commented Sep 3, 2024

Obviously it can't be landed yet, but I prepared the change for the SDK that will be needed to roll this change into the SDK:

https://dart-review.googlesource.com/c/sdk/+/383400

It does not add support for passing a second URI yet, it's just the minimum changes to the APIs that would keep things working as-is with this change (I don't want to complicate rolling a new devtools_shared into the SDK further). I'll add the additional flag for passing another URI in another change after this is done.

packages/devtools_shared/lib/src/common.dart Outdated Show resolved Hide resolved
packages/devtools_shared/CHANGELOG.md Outdated Show resolved Hide resolved
packages/devtools_shared/lib/src/common.dart Outdated Show resolved Hide resolved
packages/devtools_shared/lib/src/server/handlers/_dtd.dart Outdated Show resolved Hide resolved
@kenzieschmoll
Copy link
Member

Please merge with master to resolve the CI failures.

@DanTup DanTup added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024
@DanTup
Copy link
Contributor Author

DanTup commented Sep 6, 2024

Great, thanks!

Copy link

auto-submit bot commented Sep 6, 2024

auto label is removed for flutter/devtools/8291, due to - The status or check suite devtools_shared test has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024
@DanTup
Copy link
Contributor Author

DanTup commented Sep 6, 2024

These failures look legit - we switched to using Uri instead of string, and some of the tests use invalid URIs that won't parse as URIs. I'll fix these up next week.

@DanTup DanTup added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 9, 2024
@auto-submit auto-submit bot merged commit 5457f57 into flutter:master Sep 9, 2024
24 checks passed
@DanTup DanTup deleted the support-two-dtd-uris branch September 9, 2024 09:57
@DanTup
Copy link
Contributor Author

DanTup commented Sep 9, 2024

When a new devtools_shared is released, we'll need to apply the changes in https://dart-review.googlesource.com/c/sdk/+/383400 while rolling it in to the SDK.

@kenzieschmoll
Copy link
Member

devtools_shared 10.1.0 has been published, and you can use the fbc2f1cda3809462811ce503197b8155df77dda6 commit hash to update the devtools rev in the DEPS file.

@DanTup
Copy link
Contributor Author

DanTup commented Sep 9, 2024

Great, thanks!

@jakemac53
Copy link
Contributor

jakemac53 commented Sep 9, 2024

I commented on the commit, but note that it appears the latest version of dds that is published does not support the latest published version of devtools_shared because there was a breaking change published as a minor version. Can we get a version of dds published which works so that a pub upgrade can resolve the issue?

Or, consider retracting the latest version of devtools_shared and re-publishing as breaking, if still inside that window

@kenzieschmoll
Copy link
Member

10.1.0 was retracted. Republishing as 11.0.0: #8313

@jakemac53
Copy link
Contributor

10.1.0 was retracted. Republishing as 11.0.0: #8313

Awesome thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App release-notes-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants