Create new package for stream manager and service extensions

devoncarew commented 1 year ago

Hi - for this new package:

who will be the primary maintainer?
which team will be the backstop (own it if the primary maintainer can't)
who do we expect to use this package? ourselves? end-users? both?
what SLO will we message for the package? respond to all issues? address our own issues and opportunistically address user issues?
would this package benefit from shipping initially as a labs package? that would give us the ability to unship it in the near term if after iterating we decided that it was no longer needed

And as an FYI, the SLOs for our various publishers:

CoderDake commented 1 year ago

@devoncarew Primary maintainer: @CoderDake Backstop team: flutter/devtools Package intention: Ourselves SLO: Low, hopefully this package will remain relatively static once the development is done. Labs Package: I'm unsure what this is, but I know that this package would at least be used between between DDS and the new Dart Tooling Daemon. So I could imagine it would stick around for a while.

devoncarew commented 1 year ago

In-lining from the associated CL for posterity:

Stream Manager, Service Events How related are these two things? The service events code might be DDS specific, and the stream mana […]

For now they are related in the sense that their implementations are going to be pulled out of DDS, so that they can then be shared with the Dart Tooling Daemon.

In the purest sense they are only related in that they both would facilitate types of communication. I don't think their actual code would have much if any overlap though

It sound like this package will only be used by ourselves - between DDS and DTD? Some common interop code? Publishing it makes it easier to share the implementation?

If so, that's likely a tools.dart.dev package, published as an unlisted package, with very clear messaging in the readme that it's not a general purpose package.

Labs packages are good when we're exploring a space and don't yet know whether we're committed to a particular direction. The SLO for labs packages is essentially no support; the intention is that it'll either graduate to a more supported publisher or be discontinued.

Backstop team: flutter/devtools

cc @kenzieschmoll

natebosch commented 1 year ago

Can you elaborate on the type of code you expect to be in this package? What do you mean by "stream manager"?

CoderDake commented 1 year ago

Can you elaborate on the type of code you expect to be in this package? What do you mean by "stream manager"?

For the Service Events,
- I will be pulling the plumbing out around the registerService behaviour.
- The code will faciliate clients registering methods that other callers can trigger.
For the Stream Manager
- I will be pulling the plumbing out around stream_manager.dart
  - This code will facilitate managing streams so that clients can broadcast messages to them

Labs packages are good when we're exploring a space and don't yet know whether we're committed to a particular direction. The SLO for labs packages is essentially no support; the intention is that it'll either graduate to a more supported publisher or be discontinued.

OK thank you I will look into this 😄

bkonyi commented 1 year ago

@devoncarew the main purpose for creating this package is to share service extension and stream management code between package:dds and the new Dart Tooling Daemon (DTD), so I wouldn't consider this code to be experimental or at risk of being discontinued. I think this package belongs under the tools publisher, although we can consider leaving it unlisted.

Also, the Dart VM team (in particular, the VM service maintainers) should be the main backstop as this is functionality required for use within DDS. I've asked Dan to set the OWNERS file to reflect that both the VM and DevTools teams are owners of this package.

CoderDake commented 1 year ago

@devoncarew would you be comfortable with the service_extension_router living in pkg, given this new information?

devoncarew commented 1 year ago

Thanks; this lgtm. To paraphrase:

@CoderDake and @bkonyi as owners; VM team to backstop ownership
this will be used to share implementation between our tools; we don't expect this to be consumed by users

Using tools.dart.dev as the publisher sounds good. As we don't expect external usage I would mark the package as unlisted. Please also make sure that the readme clearly reflects the intent of the package and any SLO (just for our own usage, ...).

CoderDake commented 1 year ago

@devoncarew sounds great to me I will make sure that it gets published to tools.dart.dev. I've also upgraded the README a bit to more clearly reflect that this will be unlisted, and have LOW SLO priority :)

Can you take another look at https://dart-review.googlesource.com/c/sdk/+/322041/8 when you get a chance to check that the starting point matches your expectations now?

natebosch commented 1 year ago

the main purpose for creating this package is to share service extension and stream management code between package:dds and the new Dart Tooling Daemon (DTD)

Why is DTD unable to depend on package:dds? I'm still confused by the intended audience for this package.

devoncarew commented 1 year ago

As a summary of various discussions, this request is for two packages:

package:dart_service_protocol_shared, which will be shared logic, currently in DDS, but moved out so it can be shared between DTD and DDS (though, see Nate's question about about dtd taking a dep on dds)
package:dtd, which is a protocol client for the upcoming DTD protocol

Both are ok for the tools.dart.dev publisher. The first package we expect to have unlisted. If we expect a bunch of exploration in the APIs - for them to start in a very provisional state - I'd consider versioning them at 0.1.0. If we expect them to start fairly full-grown - or we'd have no real customers than ourselves, like dart_service_protocol_shared, starting with a major version (1.0.0) could also work.

And it sounds like you'd want dart_service_protocol_shared hosted out of <sdk>/pkg. package:dtd could go in <sdk>/pkg or dart-lang/tools, as convenient.

natebosch commented 1 year ago

package:dtd, which is a protocol client for the upcoming DTD protocol

I'd also like to see a brief discussion about how much overlap we expect in the audience for this package compared to package:vm_service_protocol.

lrhn commented 1 year ago

If we're only sharing mature code, that it's not expected to change much, if at all, then just copying the code is an option. (If we're really determined to keep the code in sync, we can make the copying part of the repository run-hooks.)

The code is not intended for anyone else to use. It's not written to be reusable, it's very specialized to the particular use, and it's very much implementation code, not APIs.

Even if it is in a shared package, we should make it clear that it's not supported for public use. We will allows presents to make breaking changes at any time, as long as our own packages can handle it. That package should not have any files outside of src/, there are no public libraries.

Which means the package will have precisely two users, ever.

Do we plan to stop supporting one of the users of this package eventually? Because at that point, the package should just be rolled into the other user, and removed as an independent package.

But, coming back to the "highly specialized implementation code", I predict that the two user packages may want to tweak the code to fit themselves better, over time. If that's the case, just sitting the code now, and letting each use evolve independently, might be better for everybody.

Just because two packages have similar needs, the code they share doesn't have to be a package. Copying at the beginning, and evolving independently, is a valid strategy, and likely better if evolution is expected. The only thing sharing specialized code is good for, is sharing bug fixes.

CoderDake commented 1 year ago

the main purpose for creating this package is to share service extension and stream management code between package:dds and the new Dart Tooling Daemon (DTD)

Why is DTD unable to depend on package:dds? I'm still confused by the intended audience for this package.

@natebosch the audience for this package will almost exclusively be internal tooling clients. Vscode, Devtools etc.

package:dds and the DTD server(which will probably be located at pkg/dtd_impl) will both depend on package:dart_service_protocol_shared , which was created to avoid duplicating a large chunk of the stream and service management code.

package:dtd will be a package that the VSCode, Devtools and other clients can use to communicate through the Dart Tooling Daemon server. It will facilitate the connection and rpc formation so the clients can just worry about passing a uri, setting up their services, and subscribing to any streams that they care about.

The overall goal of the DTD system is to have a server that's lifecycle isn't coupled to the life of a single application run. Instead it can exist as a part of the lifecycle that the tooling dictates. i.e. VSCode would start that Dart Tooling Daemon server once, and would keep it up even if the app starts/stops.

CoderDake commented 1 year ago

@lrhn that seems fair that we could just copy and go. Unfortunately most of the work has already been done here https://github.com/dart-lang/sdk/tree/main/pkg/dart_service_protocol_shared

I can't predict the future in regards to specialized changes, but the intention of DTD is much simpler and is planned to only need the stream and service behaviour. It doesn't have any of the extra complications that DDS has around being tied into the vm lifecycle etc.

bkonyi commented 1 year ago

package:dtd, which is a protocol client for the upcoming DTD protocol

I'd also like to see a brief discussion about how much overlap we expect in the audience for this package compared to package:vm_service_protocol.

There will be overlap, but not 100%. For example, the analysis server will likely never need to make a VM service connection but will want to make a DTD connection.

@lrhn that seems fair that we could just copy and go. Unfortunately most of the work has already been done here https://github.com/dart-lang/sdk/tree/main/pkg/dart_service_protocol_shared

I agree, there is a sunk cost here already and I'm not sure it's any simpler to throw it away at this point. The service extension and stream management logic is tricky to get right and has already been implemented twice (once in the VM service and once in DDS), so having one less implementation might be beneficial. It should be easy enough to remove later if we decide to.

natebosch commented 1 year ago

package:dds and the DTD server(which will probably be located at pkg/dtd_impl) will both depend on package:dart_service_protocol_shared , which was created to avoid duplicating a large chunk of the stream and service management code.

This does not answer my question. What problems are caused if pkg/dtd_impl has a dependency on package:dds, which also avoids duplicating any code?

There will be overlap, but not 100%. For example, the analysis server will likely never need to make a VM service connection but will want to make a DTD connection.

SGTM. Probably decoupling major version changes makes it worthwhile.

bkonyi commented 1 year ago

package:dds and the DTD server(which will probably be located at pkg/dtd_impl) will both depend on package:dart_service_protocol_shared , which was created to avoid duplicating a large chunk of the stream and service management code.

This does not answer my question. What problems are caused if pkg/dtd_impl has a dependency on package:dds, which also avoids duplicating any code?

pkg/dtd_impl will not depend on package:dds as the implementations of the service are different. package:dds was not written in a way that its implementation logic could easily be shared and it has some additional special cases that wouldn't apply in a DTD implementation. That's why the logic that could be shared was pulled into package:dart_service_protocol_shared.

natebosch commented 1 year ago

as the implementations of the service are different. package:dds was not written in a way that its implementation logic could easily be shared and it has some additional special cases that wouldn't apply in a DTD implementation. That's why the logic that could be shared was pulled into package:dart_service_protocol_shared.

I don't think this addresses whether there are concerns around a dependency. The code can be refactored and shared in a new package, or refactored and shared in dds. For unpublished packages we are looser around things like src/ imports.

The fe and analyzer shared dependency was created when both were published packages IIRC. Since the DTD implementation will not be published I'm still trying to understand why we need the same pattern here. What are we making easier by adding this package?

bkonyi commented 1 year ago

I don't think this addresses whether there are concerns around a dependency. The code can be refactored and shared in a new package, or refactored and shared in dds. For unpublished packages we are looser around things like src/ imports.

The fe and analyzer shared dependency was created when both were published packages IIRC. Since the DTD implementation will not be published I'm still trying to understand why we need the same pattern here. What are we making easier by adding this package?

Ah, okay, I think I understand now. That's a good point, we could instead place package:dart_service_protocol_shared under package:dds/src/service_protocol_shared and reach in there directly from DTD's implementation and avoid publishing a separate package altogether. I'm okay with that idea. WDYT @CoderDake + @devoncarew?

devoncarew commented 1 year ago

So, to paraphrase my understanding:

package:dds/stuff/... (existing dds functionality)
package:dds/src/shared/... (shared service protocol utility code)
package:dtd_impl (the unpublished DTD implementation, using some code from package:dds)
package:dtd (a published client of the DTD protocol, using none of the above packages)

Is that correct?

bkonyi commented 1 year ago

So, to paraphrase my understanding:

package:dds/stuff/... (existing dds functionality)

package:dds/src/shared/... (shared service protocol utility code)

package:dtd_impl (the unpublished DTD implementation, using some code from package:dds)

package:dtd (a published client of the DTD protocol, using none of the above packages)

Is that correct?

Yes, that's correct.

devoncarew commented 1 year ago

@CoderDake - sounds good?

dart-lang / sdk

Create new package for stream manager and service extensions #53297