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

Update to new azure-sdk-for-js #2257

Merged
merged 8 commits into from
Aug 4, 2020
Merged

Update to new azure-sdk-for-js #2257

merged 8 commits into from
Aug 4, 2020

Conversation

ejizba
Copy link
Contributor

@ejizba ejizba commented Jul 24, 2020

@ejizba ejizba requested a review from a team as a code owner July 24, 2020 18:23
@@ -80,9 +79,13 @@ function installFuncCli() {
}

async function gulp_installPythonExtension() {
return gulp_installVSCodeExtension('ms-python', 'python');
return gulp_installVSCodeExtension('ms-python', 'python', true);
Copy link
Member

Choose a reason for hiding this comment

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

Why change this to use Insiders only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Azure Account extension stuff is a part of VS Code 1.48 (see this comment), so we have to run tests on insiders until 1.48 goes stable

// tslint:disable-next-line:no-string-literal no-unsafe-any
.filter(entry => entry['m:properties']['d:IsPrerelease']['_'] === 'false')
// tslint:disable-next-line:no-string-literal no-unsafe-any
private async parseLatestAzModuleVersion(response: HttpOperationResponse): Promise<string> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is async anymore.

});

// tslint:disable-next-line:no-string-literal no-unsafe-any
if (moduleInfo['feed'] && moduleInfo['feed']['entry'] && Array.isArray(moduleInfo['feed']['entry'])) {
Copy link
Member

Choose a reason for hiding this comment

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

How come you were able to get rid of this logic completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new sdk will automatically parse xml and return it in response.parsedBody, so I changed to use that instead of manually parsing with "xml2js" ourselves. When I went to test it, the parsed object was slightly different, not entirely sure why. The new sdk still uses "xml2js" under the covers, but maybe they pass in different options?


[this._rawBindings, this._rawResources, this._rawTemplates] = <[object, object, object[]]>(
await Promise.all([bindingsRequest, resourcesRequest, templatesRequest].map(requestUtils.sendRequest))
).map(parseJson);
await Promise.all([release.bindings, resourcesUrl, release.functions].map(url => requestUtils.sendRequestWithTimeout({ method: 'GET', url })))
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it's worth condensing all of this just to look cleaner. For instance, I don't think I'd realize release.functions is the url to retrieve templates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't do it to look cleaner. Just did it because the new requestUtils only has one method (sendRequestWithTimeout) whereas the old requestUtils had two (getDefaultRequestWithTimeout and sendRequest)

} catch (error) {
if (parseError(error).errorType === 'ETIMEDOUT') {
if (parseError(error).errorType === 'REQUEST_ABORTED_ERROR') {
Copy link
Member

Choose a reason for hiding this comment

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

Why did the error type change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error type is defined by the sdk and the new sdk has different errors 🤷‍♂️ Something to watch out for in all extensions when moving to the new sdk

@ejizba ejizba merged commit df8430a into master Aug 4, 2020
@ejizba ejizba deleted the ej/sdkv2 branch August 4, 2020 21:28
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants