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

remove update-notifier dependency #91

Closed
reviewher opened this issue Jan 10, 2021 · 6 comments
Closed

remove update-notifier dependency #91

reviewher opened this issue Jan 10, 2021 · 6 comments

Comments

@reviewher
Copy link

Discussion from when the npm cli removed the dependency: https://npm.community/t/npm-version-takes-3-minutes-to-complete-when-run-in-child-process-and-behind-corporate-proxy/1658/10

It is causing local issues thanks to the .DS_Store files in the dependency tree.

@BendingBender
Copy link
Collaborator

Could you please describe in detail what exact issue do you have with update-notifier?

@reviewher
Copy link
Author

update-notifier introduces a number of problems, some of which are detailed in the linked discussion, and it isn't really adding any functionality (it merely serves as nagware).

The local problems stem from the brew MacOS system. It will scan /usr/local and proactively try to remove .DS_Store files. brew is usually invoked as the current user and may not have write access to the /usr/local/lib/node_modules NPM module cache.

In a situation like this, the obvious options are "fix update-notifier dependency" or "remove it altogether". The update notifier is only used to check module version, introducing network requests with no obvious upside to the actual functionality. IMHO removing the dependency is the better move.

@BendingBender
Copy link
Collaborator

BendingBender commented May 30, 2021

It will scan /usr/local and proactively try to remove .DS_Store files.

Excuse me, but I still didn't get it. What will scan and remove .DS_Store files? update-notifier? Have you tried to report the issue there?

@SamVerschueren This is a bit disappointing and concerning that update-notifier won't work in some corporate networks. Do you really think that this is worth it given the marginal benefits this module actually brings? It doesn't serve any core functionality of tsd but seems to sometimes interfere with correct functionality.

@reviewher
Copy link
Author

brew "macOS package manager" is trying to remove the .DS_Store file.

The actual offense is deep in the dependency tree:

└─┬ tsd@0.16.0
  └─┬ update-notifier@4.1.3
    ├─┬ boxen@4.2.0
    │ ├── term-size@2.2.1

https://unpkg.com/browse/term-size@2.2.1/vendor/ you can see the .DS_Store file exists in the term-size module under /vendor/

In a situation like this, the obvious options are "fix update-notifier dependency" or "remove it altogether".

"fix update-notifier dependency" means we would need term-size to be fixed, then boxen, then update-notifier.

The update-notifier dependency merely checks the module version number against the public registry using a network request. This request is unnecessary for the core functionality of tsd and does not honor any local npm proxy settings (it looks for the registry setting). That is probably a bug somewhere in the dependency tree but someone more knowledgeable would have to dig into it.

Once you realize the only purpose is to nag users, it's clear that removing update-notifier is the best option.

@BendingBender
Copy link
Collaborator

BendingBender commented Jun 2, 2021

Still, as much as you may dislike it, the immediate way forward to fix your bug is to fix the supply chain. This means that you'll have to create issues/pull requests in the affected repositories one by one. This will fix similar issues for other users of other projects along the way and make the whole ecosystem healthier.

You've made your point clear that you don't like update-notifier. I'm not the maintainer of this project so I don't know what stance @SamVerschueren has on this feature/misfeature. Until this is clear, please file issues with the upstream projects.

@SamVerschueren
Copy link
Collaborator

I'm ok with removing it though. Don't have a hard stance on keeping it, and if it causes trouble, then we better get rid of it. As pointed out, it's not super useful and is not needed for the core of tsd to work properly.

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

No branches or pull requests

3 participants