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

Fix arm-containerservice deprecated pacakages warning. #23

Merged
merged 3 commits into from
May 30, 2020
Merged

Fix arm-containerservice deprecated pacakages warning. #23

merged 3 commits into from
May 30, 2020

Conversation

Tatsinnit
Copy link
Member

@Tatsinnit Tatsinnit commented May 28, 2020

Following PR fixes the deprecated warning, screenshot below. in an attempt to fix this: #14

Which probably has to do with the type which get passed as target so if I do any unknown with msRestJs.ServiceClientCredentials type conversion of target.root.credentials that fixes the whole the issue.

Argument of type 'import(".../ms-rest/index").ServiceClientCredentials' is not assignable to parameter of type 'import("...@azure/ms-rest-js/es/lib/credentials/serviceClientCredentials").ServiceClientCredentials'.

With this fix, new nom install gives this

Screen Shot 2020-05-28 at 9 15 23 PM

@Tatsinnit Tatsinnit requested a review from itowlson May 28, 2020 09:22
@Tatsinnit
Copy link
Member Author

Tatsinnit commented May 28, 2020

💡 📓 The build error you see here is directly associated with : Azure/ms-rest-js#367 I tried with version below .2.0.3 for "@azure/ms-rest-js": "2.0.2" and same issue persist.

  • But inclusion of DOM in tsconfig fixes the build failure.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This looks fine to me though I would like to understand the DOM thing better. Would you be able to spare a few minutes (no longer) to look into that/explain it to me please? Thanks!

@@ -4,7 +4,8 @@
"target": "es6",
"outDir": "out",
"lib": [
"es6"
"es6",
"DOM"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems strange and I don't get why we would need DOM here (I mean I get that we need it here to make things work but I don't know why don't they work without it and is this the right fix). Is there documentation that prescribes this? If not, can we check in with the team that this is what they expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh never mind I realised you had already posted the link and oh dear yeah that's not good.

Copy link
Member Author

@Tatsinnit Tatsinnit May 28, 2020

Choose a reason for hiding this comment

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

😊 Yes, that is almost year and above when folks found the issue in “ms-rest-js” , so essentially deprecation help page should link the existing bug for folks who are fixing this deprecation to know what trick will actually fix their typing issues. 😎🙏

For those who might just read this comment here is an open bug: Azure/ms-rest-js#367

src/extension.ts Outdated Show resolved Hide resolved
@itowlson itowlson requested a review from philliphoff May 28, 2020 20:29
@itowlson
Copy link
Contributor

@philliphoff Would you mind taking a very quick look at this PR please? I think you understand the credentials and Azure Account stuff better than the rest of us do! Thanks!

@Tatsinnit
Copy link
Member Author

Tatsinnit commented May 28, 2020

This looks fine to me though I would like to understand the DOM thing better. Would you be able to spare a few minutes (no longer) to look into that/explain it to me please? Thanks!

📓 I will add few chain of thought as continuity from what I understand by reading the ms-rest-js issue.

dom lib is a set of JavaScript Web API interfaces, including DOM, DOM Events,... full source code here: https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts

For backend development, it is optional to include this. However, there is some package that is used for both frontend and backend. And you will meet type missing error when compiling without including it.

Hence this issue for us of missing types through ms-rest-js , and like one of the discussion here point: Azure/ms-rest-js#367

The root cause IIUC is that the lib entry in tsconfig is applied for an entire program and a library's tsconfig and your dependencies' tsconfigs are ignored. So any lib entries your library needs to build will have to be present in consumers' tsconfig else they will get errors.

@Tatsinnit Tatsinnit marked this pull request as ready for review May 28, 2020 23:45
@Tatsinnit Tatsinnit requested a review from itowlson May 28, 2020 23:45
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This looks good - thanks for dealing with it. Let's see if Phillip has time to take a look in the next couple of days, otherwise I'd be okay with just merging it now.

@philliphoff
Copy link
Collaborator

Assuming that the credentials returned by vscode-azure-account match those requested by the new ARM library, beyond the superficial typed definition requiring the ugly cast, then it's probably fine for now.

You're not the only extension dealing with this issue (see vscode-azure-account#140). If and when that library is finally updated, it could break this workaround.

@Tatsinnit Tatsinnit merged commit 292e4b7 into Azure:master May 30, 2020
peterbom referenced this pull request in peterbom/vscode-aks-tools Sep 13, 2023
* feed error msg to openai

* ai helper work

---------

Co-authored-by: Hariharan Subramanian <hsubramanian@microsoft.com>
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.

3 participants