-
Notifications
You must be signed in to change notification settings - Fork 28
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
☁️ 🐚 Migrate Azure Cloud Shell feature #848
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see anything obviously wrong that stands out to me.
Only thing that I don't see is the equivalent of uploadFile
being migrated over.
https://github.com/microsoft/vscode-azure-account/pull/958/files#diff-04bba6a35cad1c794cbbe677678a51de13441b7a6ee8592b7b50be1f05c6f626L112
@@ -4,14 +4,16 @@ | |||
"target": "es6", | |||
"outDir": "out", | |||
"lib": [ | |||
"es6" | |||
"es6", | |||
"DOM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. I'm not happy we have to add that but I hope to be able to remove it once I move this whole feature onto built-in fetch.
@@ -44,18 +30,18 @@ const webConfig = dev.getDefaultWebpackConfig({ | |||
'../build/default/validation': 'commonjs ../build/default/validation', | |||
'../build/Release/bufferutil': 'commonjs ../build/Release/bufferutil', | |||
'../build/default/bufferutil': 'commonjs ../build/default/bufferutil', | |||
|
|||
// required for cloud shell feature | |||
bufferutil: 'commonjs bufferutil', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the scary stuff 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don't know what this does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmao I'm pretty sure it's polyfilling. Probably has to do with cloud shell stuff running on its own node process.
|
await delay(300); | ||
|
||
for (let i = 0; i < 10; i++) { | ||
if (token !== resizeToken) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Is it possible for token
to not equal resizeToken
here?
Purpose: In order to deprecate Azure Account, we need move the Azure Cloud Shell feature to the Azure Resources extension.
I will be making updates to the code later but wanted to perform an initial migration without changing the code too much.
node-fetch
andrequest-promise
, in a future update I hope to replace both of these with built-in fetch.This feature is fairly complex and does a lot of out of the ordinary stuff to make it work. Please ask questions. I'm tentatively planning to write a document in the repo explaining the workings of the feature.
edit: Also I purposely left two prominent bugs in the code from AA. I will fix those in follow up PRs. The fixes for those are in #851, so use that for testing.