Skip to content

[eventhubs] - pass an interface to BlobCheckpointStore's constructor#20344

Merged
maorleger merged 3 commits intoAzure:mainfrom
maorleger:fix-eventhubs-dependency
Feb 11, 2022
Merged

[eventhubs] - pass an interface to BlobCheckpointStore's constructor#20344
maorleger merged 3 commits intoAzure:mainfrom
maorleger:fix-eventhubs-dependency

Conversation

@maorleger
Copy link
Copy Markdown
Member

@maorleger maorleger commented Feb 11, 2022

Packages impacted by this PR

@azure/eventhubs-checkpointstore-blob

Issues associated with this PR

Issue #13667

Describe the problem that is addressed by this PR

Because:

  • BlobCheckpointStore's constructor takes a class (ContainerClient) as an argument
  • That class is defined in @azure/storage-blob
  • ContainerClient contains private members

You cannot pass what would otherwise be a compatible ContainerClient instance to
BlobCheckpointStore unless it is successfully deduped by NPM, even across minor
versions.

This causes compatibility issues for anyone who upgrades storage-blob.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

I tried to get clever with mapped types, but ran into a few situations (These examples probably have a lot of mistakes, it was hard to keep them all in-place):

// Doesn't work because `getBlobClient` returns a class with private members as
// well so we need to recursively map return values to Public
type Public<T> = Pick<T, keyof T>;
type ContainerClientLike = Public<ContainerClient>

// Doesn't work because of PagedAsyncIterableIterator...
// Type 'Public<PagedAsyncIterableIterator<BlobItem, ContainerListBlobFlatSegmentResponse, PageSettings>>' must have a '[Symbol.asyncIterator]()' method that returns an async iterator.
type Public<T> = {
  [P in keyof T]: T[P] extends (...args: infer Params) => infer ReturnValue
    ? (...args: Params) => Public<ReturnValue>
    : T[P];
};

// Doesn't work because TypeScript wants to narrow one of the nested functions
// as such:
/*
        Types of property 'isDone' are incompatible.
          Type '() => boolean' is not assignable to type '(() => false) | (() => true)'.
            Type '() => boolean' is not assignable to type '() => false'.
              Type 'boolean' is not assignable to type 'false'.ts(2345)
*/
type Public<T> = {
    [P in keyof T]: T[P] extends (...args: infer Params) => infer ReturnValue
      ? ReturnValue extends Promise<infer PReturnValue> // for promises we need to return a promise of the public shape
        ? (...args: Params) => Promise<Public<PReturnValue>>
        : (...args: Params) => Public<ReturnValue> // otherwise we reutnr the public shape
      : T[P]; // not a function, so just the public property
  };

// Even if I was to get past this, I'd also have to map the _parameters_ of every function...

At which point I gave up and just copied the minimal interface needed to support
our calls

Are there test cases added in this PR? (If not, why?)

No, the existing test cases already cover assignability

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@azure-sdk
Copy link
Copy Markdown
Collaborator

API changes have been detected in @azure/eventhubs-checkpointstore-blob. You can review API changes here

@jeremymeng
Copy link
Copy Markdown
Member

// Doesn't work because `getBlobClient` returns a class with private members as
// well so we need to recursively map return values to Public
type Public<T> = Pick<T, keyof T>;

Does Public<T> explode during runtime? I am wondering whether type ContainerClientLike = Pick<ContainerClient, "getBlobClient" | "listBlobsFlat"> work...

@maorleger
Copy link
Copy Markdown
Member Author

// Doesn't work because `getBlobClient` returns a class with private members as
// well so we need to recursively map return values to Public
type Public<T> = Pick<T, keyof T>;

Does Public<T> explode during runtime? I am wondering whether type ContainerClientLike = Pick<ContainerClient, "getBlobClient" | "listBlobsFlat"> work...

Doesn't quite work, and the reason is that by returning getBlobClient which itself returns a class we end up with the same scenario, only one level deep.

But that gave me an idea! this seems to work:

import { ContainerClient, BlobClient, BlockBlobClient } from "@azure/storage-blob";

export type BlobClientLike = {
  [P in keyof Pick<BlobClient, "getBlockBlobClient">]: BlobClient[P] extends (
    ...args: Parameters<BlobClient[P]>
  ) => BlockBlobClient
    ? (...args: Parameters<BlobClient[P]>) => BlockBlobClientLike
    : never;
};

export type ContainerClientLike = {
  [P in keyof Pick<ContainerClient, "getBlobClient" | "listBlobsFlat">]: P extends "getBlobClient"
    ? (...args: Parameters<ContainerClient[P]>) => BlobClientLike
    : ContainerClient[P];
};

export type BlockBlobClientLike = Pick<BlockBlobClient, "upload" | "setMetadata">;

But I can't decide which I like better 😄 I do have to clean this up a bit

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Even though there's no fancy type math, I like the approach shown in this PR for abstracting the interfaces. We did similar work in the past with PipelineLike and WebResourceLike so there is plenty of prior art.

@maorleger
Copy link
Copy Markdown
Member Author

/azp run js - eventhubs-checkpointstore-blob - tests

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@maorleger maorleger enabled auto-merge (squash) February 11, 2022 22:46
@maorleger maorleger merged commit af5fdde into Azure:main Feb 11, 2022
@ramya-rao-a ramya-rao-a mentioned this pull request Feb 14, 2022
4 tasks
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.

4 participants