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

[cli] upgrade update-notifier to fix vulnerability #8038

Closed
wants to merge 2 commits into from
Closed

[cli] upgrade update-notifier to fix vulnerability #8038

wants to merge 2 commits into from

Conversation

alex-grover
Copy link

Description

Upgrade update-notifier for security vulnerability fixed in yeoman/update-notifier#222. An older version is included via ava but I didn't touch it since that's a dev dep.

The unit tests pass but seem to modify the fixture file so I included that as well.

Tests

  • The code changed/added as part of this PR has been covered with tests
  • All tests pass locally with yarn test-unit

Code Review

  • This PR has a concise title and thorough description useful to a reviewer
  • (N/A) Issue from task tracker has a link to this PR

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

This version switched to ESM so we'll need to update TS as well so we can use await import() from CJS

https://github.com/yeoman/update-notifier/blob/3b6b9b18428aa3848960f02df53cb571a7620a51/package.json#L13

I'm starting to wonder if it makes more sense to drop update-notifier since its quite large from something that should be a single fetch.

@alex-grover
Copy link
Author

@styfle got it, thanks for the pointers! happy to take a stab at upgrading TS, let me know how you'd like to proceed.

@alex-grover
Copy link
Author

closing in favor of #8090

cb1kenobi added a commit that referenced this pull request Dec 1, 2022
This PR replaces the `update-notifier` dependency with a custom
implementation.

There are a few reasons: the dependency is quite large, it requires ESM
in order to update, and can sometimes suggest an update to an older
version. For example:


![image](https://user-images.githubusercontent.com/229881/195891579-c8c047a6-51ec-45f2-b597-daf927f48203.png)


- Related to #8038

Co-authored-by: Chris Barber <chris.barber@vercel.com>
Co-authored-by: Chris Barber <chris@cb1inc.com>
Co-authored-by: Sean Massa <EndangeredMassa@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.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.

None yet

2 participants