Skip to content

Decoupling LRO from core-client#1043

Merged
deyaaeldeen merged 11 commits intoAzure:mainfrom
deyaaeldeen:lro-abstract
Jul 6, 2021
Merged

Decoupling LRO from core-client#1043
deyaaeldeen merged 11 commits intoAzure:mainfrom
deyaaeldeen:lro-abstract

Conversation

@deyaaeldeen
Copy link
Copy Markdown
Member

@deyaaeldeen deyaaeldeen commented Jun 17, 2021

This PR refactors the LRO files so that all files in src/lro do not use anything from core-client. These files are being instantiated with core-client things in src/coreClientLro.ts. A similar file for core-http is provided, src/coreHttpLro.ts which restores the LRO support for core-http clients. This PR is a step toward moving everything in src/lro to @azure/core-lro. See Azure/azure-sdk-for-js#15949 for details.

@deyaaeldeen deyaaeldeen requested a review from joheredi June 17, 2021 22:44

export function processAzureAsyncOperationResult<TResult>(
restrieveResource: (path?: string) => Promise<LROResult<TResult>>,
lro: LRO<TResult>,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

passing the whole object is needed so retrieveAzureAsyncResource can access this internaly.

}
}

export class CoreClientLRO<T> implements LRO<T> {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this class is the main contribution of this PR

Comment on lines +977 to +980
`const lro = new CoreClientLRO(sendOperation,${operationParamsName},
${operationSpecName},${finalStateStr})`,
`return new LROPoller({intervalInMs: options?.updateIntervalInMs},
${operationParamsName},
${operationSpecName},
sendOperation,
${finalStateStr}
lro
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is where CoreClientLRO is instantiated.

/**
* Description of a long running operation
*/
export interface LRO<T> {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is the abstract interface to be instantiated by any client

Comment on lines +136 to +142
export function extractHttpDetails({
path,
method
}: OperationRequestDetails): {
path: string;
httpMethod: HttpMethods;
} {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for some reason the compiler did not like leaving the return type implicit so I had to add it.

@deyaaeldeen deyaaeldeen marked this pull request as ready for review July 2, 2021 00:08
@deyaaeldeen deyaaeldeen requested a review from sarangan12 as a code owner July 2, 2021 00:08
@sarangan12
Copy link
Copy Markdown
Contributor

The changes look fine. But, in our last conversation, Jose was telling me that the plan was to move LROEngine to core-lro. This PR does not seem to do that. Or is it some intermediate step? Can you clarify?

@deyaaeldeen
Copy link
Copy Markdown
Member Author

@sarangan12 sorry about the confusion! Yes it is an intermediate step. I updated the PR description.

@deyaaeldeen deyaaeldeen merged commit 114ea11 into Azure:main Jul 6, 2021
@deyaaeldeen deyaaeldeen deleted the lro-abstract branch July 6, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants