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

chore(deps): switched from request to @theia/request #12413

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

kittaakos
Copy link
Contributor

What it does

This PR replaces request with @theia/request to eliminate all dependency vulnerabilities detected by yarn audit.

% yarn audit
yarn audit v1.22.18
0 vulnerabilities found - Packages audited: 1912
✨  Done in 1.35s.

How to test

  • CI should pass, and
  • yarn audit should produce the 0 vulnerabilities found result

Closes #12401

Review checklist

Reminder for reviewers

I have no idea how to test/verify the HTTP and GitHub plugin resolvers 😕

@kittaakos
Copy link
Contributor Author

Can somebody please advise what to do to fix the license check? Thank you!

X npm/npmjs/-/nwsapi/2.2.4, 
ERROR: Found results that aren't part of the baseline!
X npm/npmjs/@types/jsdom/21.1.1, 

@kittaakos
Copy link
Contributor Author

I have made the requested changes and downgraded to jsdom@17. The 3pp check still fails with the following error:

ERROR: Found results that aren't part of the baseline!
X npm/npmjs/-/nwsapi/2.2.4, 

nwsapi has MIT license. What's the procedure to pass the 3pp check? Thanks!

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

yarn audit no longer warns about vulnerabilities.

dev-packages/application-package/src/npm-registry.ts Outdated Show resolved Hide resolved
@paul-marechal
Copy link
Member

@kittaakos there's not much you can do about new dependencies that cause dash-license to fail but @vince-fugnitto ran the tool in auto-review mode on your PR to make the check go green.

@kittaakos
Copy link
Contributor Author

@kittaakos there's not much you can do about new dependencies that cause dash-license to fail but @vince-fugnitto ran the tool in auto-review mode on your PR to make the check go green.

If the PR does not require a CQ, I would bump jsdom to 22+ instead of 17. Let me know what you think. Thanks!

@vince-fugnitto
Copy link
Member

If the PR does not require a CQ, I would bump jsdom to 22+ instead of 17. Let me know what you think. Thanks!

@kittaakos I'm fine with bumping to the latest if it works well, I'll perform a dash-licenses review as necessary.

@kittaakos
Copy link
Contributor Author

I'm fine with bumping to the latest if it works well, I'll perform a dash-licenses review as necessary.

I bump to the latest jsdom and push it. The tests pass. I push the changes in a few minutes. Thanks!

@kittaakos
Copy link
Contributor Author

I bump to the latest jsdom and push it.

It's done.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Apr 19, 2023

@paul-marechal
Copy link
Member

I wanted to merge but GH tells me there are conflicts? Can you look into it? I'll merge then.

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>
@kittaakos
Copy link
Contributor Author

kittaakos commented Apr 20, 2023

Can you look into it? I'll merge then.

Thanks! I rebased the PR from the master branch and pushed the changes. I had to force-push it 😕

@msujew
Copy link
Member

msujew commented Apr 20, 2023

@kittaakos The branch is out of date, so we can't merge it. Can you allow edits to your PR from collaborators, so that we can rebase the branch ourselves before merging?

@kittaakos
Copy link
Contributor Author

Can you allow edits to your PR from collaborators

Done. Thanks for taking care of it. Great support from all of you 👏

This was referenced Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Replace request with an alternative library
4 participants