Switch to axios to support proxy server when merging. - #1190
Switch to axios to support proxy server when merging.#1190Emilio Munoz (munozemilio) merged 5 commits into
Conversation
Sorry, something went wrong.
Normalize sorting to reduce variation.
| const filePrefix = 'file:///' | ||
|
|
||
| // Get JSON from a URL. | ||
| export default async function getJSON(url: string): Promise<any> { |
There was a problem hiding this comment.
async function getJSON<T = any>(url: string): Promise<T> { ... } allows some flexibility without forcing everyone to accept any. You may have to add return resp.data as T as well.
Sorry, something went wrong.
There was a problem hiding this comment.
The purpose of this function is to return JSON. Why do we need the flexibility?
In reply to: 612850256 [](ancestors = 612850256)
Sorry, something went wrong.
| */ | ||
|
|
||
| import * as fs from 'fs-extra' | ||
| const axios = require('axios') |
There was a problem hiding this comment.
It seems odd to mix and match import/require. Is there a convention here Emilio Munoz (@munozemilio)?
Sorry, something went wrong.
There was a problem hiding this comment.
Generally speaking I do import when TS and otherwise do require. Happy if there is a better pattern.
In reply to: 612850391 [](ancestors = 612850391)
Sorry, something went wrong.
| "@types/xml2js": "^0.4.4", | ||
| "ajv": "^6.12.2", | ||
| "axios": "~0.21.1", | ||
| "axios-https-proxy": "^0.1.1", |
There was a problem hiding this comment.
There is a warning thrown for the same implementation in LU: npm WARN axios-https-proxy@0.1.1 requires a peer of axios@^0.18.0 but none is installed. You must install peer dependencies yourself.
Please sync with Shuai Wang (@cosmicshuai) since the same fix will be done in LU #Resolved
Sorry, something went wrong.
There was a problem hiding this comment.
Synced will Shuai Wang (@cosmicshuai), we had a cleaner way to remove this dependency and the warning. Shuai Wang (@cosmicshuai) how about you commit to this PR for this? #Resolved
Sorry, something went wrong.
There was a problem hiding this comment.
removed axios-https-proxy and commit a change to handle the https proxy setting locally. #Resolved
Sorry, something went wrong.
Emilio Munoz (munozemilio)
left a comment
There was a problem hiding this comment.
CIL
Sorry, something went wrong.
|
Emilio Munoz (@munozemilio) I modified the code, and removed the dependency of axios-https-proxy. It will no longer throwing this warning. |
Sorry, something went wrong.
Shuai Wang (@cosmicshuai) Where is the "Accessing non-existing property" warning coming from? |
Sorry, something went wrong.
That's comming from anltr on node14, which Shuai is on. |
Sorry, something went wrong.
* Switch to axios to support proxy server when merging. Normalize sorting to reduce variation. * modify https proxy setting * refactor * fix lint * fix lint Co-authored-by: Chris McConnell <chrimc> Co-authored-by: cosmicshuai <shuai_uf@outlook.com>
* Switch to axios to support proxy server when merging. (#1190) * Switch to axios to support proxy server when merging. Normalize sorting to reduce variation. * modify https proxy setting * refactor * fix lint * fix lint Co-authored-by: Chris McConnell <chrimc> Co-authored-by: cosmicshuai <shuai_uf@outlook.com> * Update orchestrator-core reference to 4.13.0 + fix for orchestrator:add (#1197) * Update package.json * Update datasourcehelper.ts * Update add.ts * Update pnpm-lock.yaml * Update botbuilder-lg to 4.13.0 (#1198) Co-authored-by: Chris McConnell <chrimc@microsoft.com> Co-authored-by: cosmicshuai <shuai_uf@outlook.com> Co-authored-by: Tien Suwandy <tiens@microsoft.com>
Normalize sorting to reduce variation.