update-notifier icon indicating copy to clipboard operation
update-notifier copied to clipboard

Run in child process instead of lazy loading every dependency

Open fregante opened this issue 2 years ago • 4 comments

Not a big fan [of lazy loading], but I've seen update-notifier as a require time offender when profiling various things. The idea of update-notifier is that it should have no impact on the consumer. It's just an added bonus, not critical. https://github.com/yeoman/update-notifier/pull/82#issuecomment-225636213

I may be wrong, it seems to me that this change was made simply to hide the package from the very initial load but in practice every dependency is still loaded right after.

update-notifier already uses a child process to do the checking, so I think it'd be best to do the whole thing there.

Related:

  • https://github.com/yeoman/update-notifier/issues/80
  • https://github.com/yeoman/update-notifier/pull/82

fregante avatar Jul 26 '21 08:07 fregante

There are some downsides with this though:

  • Risk of breaking something. update-notifier is quite stable now.
  • The TTY check will not work correctly, so it will need special handling.

What are the upsides? Slightly cleaner code?

I also wonder if using a worker thread would be easier. We don't really need to separation a child process provides. We just want the checking to not impact the startup performance of the Node.js app/CLI.

sindresorhus avatar Jul 26 '21 12:07 sindresorhus

If I understand this correctly, the whole loading still happens in the main thread, so this does affect the loading, it's just that those tools don't pick it up.

If, as is common, code is run asynchronously, then update-notifier eventually competes for the same resources as the main app (because the lazy import is scheduled together with the app's own async callbacks)

The only advantage of the current situation is that update-notifier is mostly removed from the very first loop, but then it still runs right after, and probably not as efficiently either (assuming that async imports have some overhead over just parsing a native import statement)

I get that it's best not to rock the boat, but… I also don't appreciate this package secretly slowing down every bin I ever run.

fregante avatar Feb 09 '23 19:02 fregante

Dependencies are no longer lazy loaded.

sindresorhus avatar Feb 11 '23 08:02 sindresorhus

I'm open to making everything done in a child process.

sindresorhus avatar Feb 11 '23 08:02 sindresorhus