[core-util] depend on @typespec/ts-http-runtime

[timovv]

Packages impacted by this PR

• @azure/core-util
• @typespec/ts-http-runtime

Issues associated with this PR

• [core] Deduplicate code between @azure/core-* and @typespec/ts-http-runtime #32698

Describe the problem that is addressed by this PR

Opening this PR to get initial feedback on the approach, not looking to get it merged immediately. Let me know your thoughts.

This is a proof of concept of how we might deduplicate the Azure and unbranded package without changing the Azure API surface in any meaningful way. This is done by exporting the necessary internals from the unbranded package under a subpath export, __internal/<package-name>. The Azure package then wraps these internals and re-exports them. All the internals exported are prefixed with a double underscore to prevent a naming conflict when they are imported by the Azure package, and also to help make sure that none of the internal names show up in the public API of the Azure package.

Exporting under the __internal/<x> subpath keeps the main API surface of @typespec/ts-http-runtime clean and focused on the library's core purpose, which is to provide the necessary APIs for code generated from TypeSpec. This necessary surface is much smaller than what the combined Azure Core packages expose today.
 
I'm starting with core-util since it's one of the simpler packages to convert. This is because:

• there are no differences between Azure and unbranded, and
• it has no complex dependencies between Core packages (like for example core-client-rest has a dependency on core-rest-pipeline).

Other packages are a bit trickier but I've had some initial success with this approach even with these differences.

(more detailed description to come; let me know if any questions!)

[azure-sdk]

API change check

API changes are not detected in this pull request.

[jeremymeng]

is there any drawback of re-exporting?

export {
  __calculateRetryDelay as calculateRetryDelay
} from "@typespec/ts-http-runtime/__internal/util";

[maorleger]

I think this is a reasonable approach, I have a few concerns, but still digesting the PR 😄

Listing them here so that conversations can be threaded

Initial concerns

Coupling

I worry that we're still tightly coupling the two. If I wanted to add myCoolHelper to core so that many client libraries can use it I now have to:

1. Add it to ts-http-runtime
2. Export it under the __internal namespace
3. Re-import it here and redefine (or re-export)
4. Export it from here
5. Use it in my client library

That's a lot of steps and will likely lead to folks skipping ts-http-runtime entirely and just adding it to core under the "it's probably azure only" rationalization

internals but not really

I know we can say "don't use internals" but Hyrum's law always applies - people will discover and use things under the __internal subpath export. And while that may be fine if we agree that it's still considered public API surface and held to the same standard I worry that we might handwave our way to making breaking changes here and there because it's "internal"

Depending on azure-core vs ts-http-runtime

Is the end goal for all azure client libraries to import from @azure/core or is the end goal for client libraries to depend on @typespec/ts-http-runtime with azure core providing additional azure-specific functionality on top of it? If the former this would work but I'm curious why that is a goal. I remember when we had @azure/core-http and while it had a large surface area it's not something that most Azure SDK customers have to deal with. Even in the TypeSpec 3p case most of the usage would be codegen'd so a large surface area is not the end of the world

Alternatives

Work outside in

I get wanting to start with core-util as it is simplest to reason about and its probably an approach I'd have taken as well, but maybe if we work outside in (starting with the core-client) we could then have azure core take a true dependency on the options, createClient etc - the API will be minimal and while core-util will remain duplicated for the beginning we may be able to better reason about it once we have a true "onion" (azure core depends on http-runtime's functionality in a meaningful way, actually using the http-runtime version but still hiding the dependency) and can tease apart what is azure-core and what is ts-http-runtime.

Treat these as public API

If we want to end up where everything relies on ts-http-runtime and azure-core contains only azure specific bits, we can continue the subpath export approach but removing the __ and the internal namespace - treat them as they are (public API) and use subpath exports to organize them

Happy to discuss further, this is just my monday morning observations and can be totally off base 😄

[timovv]

Thanks for the feedback. Let me try and explain where I'm coming from a little more.

As you know, at the moment we have what is essentially two copies of the same code, with minor differences, in the Azure and unbranded Core packages. This is really not a place we want to be in long-term, since it's so easy for the two codebases to unintentionally diverge. This has already happened a few times. Having to keep everything in sync is risky and a time sink. The goal of this part of the work is to remove the duplication by re-exporting what we can from the unbranded library in the Azure libraries. The fact of the matter is that we will have to continue to support the Azure Core libraries with the current API surface for quite some time. This initial deduplication effort is really about minimizing the effort required to keep this support going -- less duplicate code means lower risk of introducing bugs and less code for us to maintain.

As a long-term vision, I would like to see the Azure client libraries depend on the unbranded package directly, with the Azure-specific parts required being in a "companion" package. But something like that will be some time away from happening and we'll still need to provide the Azure Core packages as they are today while we still have libraries that use them.

Coupling

Point taken -- yes, there's still very tight coupling between the Azure and unbranded libraries. I don't know if there's much that can be done about that in the short term if we need to keep shipping Azure Core in its current form. But this is surely better than the status quo, which involves basically copy-pasting whatever you implement into two places.

"it's probably azure only" rationalization

I actually don't think that this is strictly a bad thing. If something can live in the Azure-only area (e.g. it's used only in our convenience layer) and we don't need it in unbranded then I think it's sensible for it to live in the Azure Core package, and it would be good to get people thinking about that when they decide to implement a Core feature. If something starts as Azure-only and we find a use for it in the unbranded package, we can always promote it. In fact, there are probably a few utils in core-util which are only used in the convenience layer of our existing SDKs, so it might be a worthwhile exercise to move those out of unbranded and back to Azure.

And for things like bug fixes which don't change the API surface, this refactor reduces the amount of work since there is now only one place you have to change the code.

internals but not really

This is a good point and something I was struggling with a little. A couple of things that I was thinking when I decided to mark everything in here as "internal" were:

• I would like to make a distinction between the parts of the surface that are necessary for codegen versus what we're exporting for our convenience so we can continue to ship Azure Core off of the same code. I think this separation will be helpful for us conceptually: what are we exposing that is actually required by the generated code, versus what are we exposing for the purposes of keeping the existing Azure Core API surface alive? A benefit of keeping this distinction is that if we ever get to the point where we don't feel the need to support the Azure Core libraries as they are today (e.g. if we get the "companion library" type approach fully working) then it will be technically easy to do a major bump and drop the internals.
• We would still treat the "internals" as public API. We have to do this anyway, since otherwise we would risk breaking the Azure Core packages that depend on them. The main idea I had with making everything "internal" and using the __ and everything was for it to be a signal: sure, folks can depend the internals if they like and that's just fine, but we're being clear about what our reasoning was for exporting that part of the API surface. Any breaking change in API or behavior, in the internals or otherwise, would be accompanied by a major version bump.
• Prefixing everything with __ was for a couple of reasons. The main one being developer (and by that I mean "our team") convenience and avoiding name conflicts. It's less obvious with core-util where we can more-or-less re-export everything without changes, but for libraries where there are differences between Azure and Unbranded e.g. core-client and core-rest-pipeline, the Azure library sometimes needs to do a little work to wrap the unbranded function while maintaining existing behavior. And the unbranded internal function may need to take in extra flags, options, and dependencies which vary depending on whether it's being called in the Azure or unbranded context. Marking the internal function with __ helps to make it clear which function is the "internal" version versus the public version (at the very least, making this distinction was helpful to me while I was playing with how we might approach this for the other Core libraries, and why not be consistent?). But not 100% on doing this if folks are strongly against.

Maybe the word "internal" here is problematic? Would something like "primitives" or something different feel better? Or "core" even?

Depending on azure-core vs ts-http-runtime

This kind of gets at the long-term vision idea I mentioned above -- the ideal end result in my view is definitely having our client libraries depending on the unbranded package with an Azure companion library. But for the time being we do need to keep shipping the existing Azure Core surface and if we can minimize the amount of duplication between the two Core flavors then I think that would make our lives easier.

Work outside in

I think it's an interesting idea. But something that I'm concerned about here is dependency issues. Many libraries that depend on @azure-rest/core-client also take dependencies on other Core packages, e.g. Document Intelligence also depends on core-rest-pipeline and core-auth. If we went with this approach I think we could get some strange behavior. For example, this helper in DI throws a core-rest-pipeline RestError. But if we migrated core-client only over to the Unbranded package, any errors thrown by it would be unbranded RestErrors. Now, maybe customers shouldn't be relying on instanceof when identifying whether the error is a RestError, but if they were doing that, that's something that might not work as expected anymore. Starting this migration at the bottom with the Core packages that don't depend on anything else and then building up from there will help with this issue I think.

Let me know if there's anything I can elaborate on with that? Happy to chat offline too

[maorleger]

Thanks for the thoughtful reply as always! Overall the missing piece for me I think was the long-term vision. Once you clarified that this is a stepping stone to get there I feel better about the approach. I'm happy to brainstorm or think about alternatives if you'd like but I would really love to hear from the rest of the team as well at some point.

As a long-term vision, I would like to see the Azure client libraries depend on the unbranded package directly, with the Azure-specific parts required being in a "companion" package. But something like that will be some time away from happening and we'll still need to provide the Azure Core packages as they are today while we still have libraries that use them.

Got it, thanks! Sometimes it's hard to tell the long-term vision from the initial work so this helps a lot and is the direction I was imagining as well 👍

Point taken -- yes, there's still very tight coupling between the Azure and unbranded libraries. I don't know if there's much that can be done about that in the short term if we need to keep shipping Azure Core in its current form. But this is surely better than the status quo, which involves basically copy-pasting whatever you implement into two places.

Yeah it's definitely a huge improvement over the status quo - no argument there!

I actually don't think that this is strictly a bad thing. If something can live in the Azure-only area (e.g. it's used only in our convenience layer) and we don't need it in unbranded then I think it's sensible for it to live in the Azure Core package, and it would be good to get people thinking about that when they decide to implement a Core feature. If something starts as Azure-only and we find a use for it in the unbranded package, we can always promote it. In fact, there are probably a few utils in core-util which are only used in the convenience layer of our existing SDKs, so it might be a worthwhile exercise to move those out of unbranded and back to Azure.

Yeah, fair! I originally was concerned about this being used an excuse to avoid exporting, versioning, and re-exporting but you're right that if it's truly necessary by both flavors then it would have to live in unbranded. So I can dismiss that thought 😄

Maybe the word "internal" here is problematic? Would something like "primitives" or something different feel better? Or "core" even?

Yeah I think calling it something other than internal and avoiding the double-underscore prefix (if possible) would help me feel a little better - I don't want future us to get complacent because something is __internal etc.

But if we migrated core-client only over to the Unbranded package, any errors thrown by it would be unbranded RestErrors. Now, maybe customers shouldn't be relying on instanceof when identifying whether the error is a RestError, but if they were doing that, that's something that might not work as expected anymore

To be fair that is an issue today as well - it's part of the reality of side-by-side package installs...

  "dependencies": {
    "core-pipeline-1": "npm:@azure/core-rest-pipeline@1.18.0",
    "core-pipeline-2": "npm:@azure/core-rest-pipeline@1.18.1"
  }

const crp1 = require("core-pipeline-1");
const crp2 = require("core-pipeline-2");

const error = new crp1.RestError("foo");

console.log(error instanceof crp2.RestError); // false

So I would not worry too much about that. I'm showing the above by purposely depending on two versions of crp but this could happen inadvertently just as well.

[jeremymeng]

I like our granularity of current core packages which allows us to iterate and publish in one area fast without worrying too much about impacting others. I know some languages are also looking into making their core more granular. Hope we will have some guidelines/rules/conventions that enable us to continue doing that.