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

Replace request with an alternative library #12401

Closed
kittaakos opened this issue Apr 12, 2023 · 4 comments · Fixed by #12413
Closed

Replace request with an alternative library #12401

kittaakos opened this issue Apr 12, 2023 · 4 comments · Fixed by #12413
Labels
dependencies pull requests that update a dependency file quality issues related to code and application quality

Comments

@kittaakos
Copy link
Contributor

Replace request with an alternative library. request is deprecated and causing yarn audit warnings. A list of alternatives is here: request/request#3143 (comment).

% yarn why request
yarn why v1.22.18
[1/4] 🤔  Why do we have the module "request"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "request@2.88.2"
info Reasons this module exists
   - "_project_#@theia#application-package" depends on it
   - Hoisted from "_project_#@theia#application-package#request"
   - Hoisted from "_project_#@theia#plugin-dev#request"
   - Hoisted from "_project_#@theia#plugin-ext-vscode#request"
   - Hoisted from "_project_#@theia#plugin-ext#request"
   - Hoisted from "_project_#jsdom#request"
info Disk size without dependencies: "696KB"
info Disk size with unique dependencies: "2.08MB"
info Disk size with transitive dependencies: "6.69MB"
info Number of shared dependencies: 37
✨  Done in 1.08s.

Since jsdom depends on an old request version, bumping jsdom would be also required:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ Server-Side Request Forgery in Request                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ request                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ No patch available                                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jsdom                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jsdom > request                                              │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1091459                     │
└───────────────┴──────────────────────────────────────────────────────────────┘

jsdom should be replaced anyway, so this would be a good opportunity to get rid of warnings:

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ moderate      │ Insufficient Granularity of Access Control in JSDom          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ jsdom                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=16.5.0                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jsdom                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jsdom                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/1089185                     │
└───────────────┴──────────────────────────────────────────────────────────────┘

Once request is gone and a more recent version of jsdom is used, one has to update the test helper here, and handle 'undefined' property:

(global as any)[property] = (dom.window as any)[property];

Feature Description:

AC: yarn audit should not generate warnings.

@vince-fugnitto vince-fugnitto added quality issues related to code and application quality dependencies pull requests that update a dependency file labels Apr 12, 2023
@vince-fugnitto
Copy link
Member

@kittaakos are you interested in contributing a fix?

@kittaakos
Copy link
Contributor Author

It will take some time, but I will take care of it. Thanks

I am more than open to any lib replacement recommendations, so if anybody has a preference, please add it with a why you recommend it as a comment.

@msujew
Copy link
Member

msujew commented Apr 12, 2023

@kittaakos We have our own @theia/request package. It might make sense to use that. It features a few advanced features, such as proxy detection and so on.

@kittaakos
Copy link
Contributor Author

We have our own @theia/request package.

OK. Let's use that. I checked the node implementation, and it comes with all required env variable support: HTTPS_PROXY and others.

kittaakos pushed a commit to kittaakos/theia that referenced this issue Apr 13, 2023
Closes eclipse-theia#12401

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Apr 13, 2023
Closes eclipse-theia#12401

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos pushed a commit to kittaakos/theia that referenced this issue Apr 13, 2023
Closes eclipse-theia#12401

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
kittaakos added a commit to kittaakos/theia that referenced this issue Apr 18, 2023
kittaakos added a commit to kittaakos/theia that referenced this issue Apr 19, 2023
kittaakos pushed a commit to kittaakos/theia that referenced this issue Apr 20, 2023
to remove the deprecated `request` dependency

 - Updated to `jsdom@21.1.1`
 - Updated to `@types/jsdom@21.1.1`

Closes eclipse-theia#12401

Co-authored-by: Mark Sujew <mark.sujew@typefox.io>
Co-authored-by: Paul Maréchal <paul.marechal@ericsson.com>
Co-authored-by: Akos Kitta <a.kitta@arduino.cc>

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
vince-fugnitto added a commit to kittaakos/theia that referenced this issue Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file quality issues related to code and application quality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants