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

[Storage] Imports @azure/abort-controller - storage-queue #4359

Merged
merged 15 commits into from
Aug 1, 2019

Conversation

HarshaNalluru
Copy link
Member

@HarshaNalluru HarshaNalluru commented Jul 19, 2019

Update storage-queue to use the @azure/aborter-controller library and remove the existing Aborter from the src folder, updates tests, recordings, changelog accordingly.

@HarshaNalluru HarshaNalluru added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Jul 19, 2019
@HarshaNalluru HarshaNalluru self-assigned this Jul 19, 2019
Copy link
Member

@XiaoningLiu XiaoningLiu left a comment

Choose a reason for hiding this comment

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

Looks good. In master branch, storage SDKs depend on "ms-rest-js". Now ms-rest-js updated to "2.0.0" which incroduces AborterSignal breaking changes. See this https://github.com/Azure/ms-rest-js/blob/master/Changelog.md#200---2019-06-21 In next storage feature release from master branch, I'm going to update to @azure/ms-rest-js@v2 version.

Any plan to align with latest @azure/ms-rest-js in @azure/core-http? Otherwise, there will be conflicts when merging master and storage/feature branch toghether.

@HarshaNalluru
Copy link
Member Author

OMG. Thanks for the heads up.
@XiaoningLiu, is it necessary to update the storage packages in the master branch to use @azure/ms-rest-js@v2?

In case that is necessary.. @daviwil, is there any plan to align with the latest @azure/ms-rest-js in @azure/core-http?

@daviwil
Copy link
Contributor

daviwil commented Jul 19, 2019

Yep, I need to pull over those changes for preview.2. I'll look into whether I can get that done tomorrow.

@XiaoningLiu
Copy link
Member

Yep, I need to pull over those changes for preview.2. I'll look into whether I can get that done tomorrow.

@HarshaNalluru ms-rest-js@v2 dropped axios and updated to node-fetch due to axios's long response for issues like proxy support. ms-rest-js@v2 also supports keep-alive which will help improve storage SDK's performance especially for tiny requests.

Due to ms-rest-js@v2's AborterSignal interface breaking change (seems in order to align with AbortSignal standards), @azure/abort-controller needs to take these changes too.

@@ -3,7 +3,7 @@

import { TokenCredential, isTokenCredential, isNode } from "@azure/core-http";
import * as Models from "./generated/lib/models";
import { Aborter } from "./Aborter";
import { AbortSignal } from "@azure/abort-controller";
Copy link
Contributor

Choose a reason for hiding this comment

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

In our other SDKs we're using the AbortSignalLike interface from @azure/abort-controller as the type for abortSignal, instead of the AbortSignal class. This is so customers can also use the native AbortController/AbortSignal built into modern web-browsers as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@HarshaNalluru HarshaNalluru requested a review from chradek July 23, 2019 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants