Run in child process instead of lazy loading every dependency

yeoman / update-notifier

Update notifications for your CLI app

BSD 2-Clause "Simplified" License

1.76k stars 132 forks source link

Run in child process instead of lazy loading every dependency #210

Open fregante opened 3 years ago

fregante commented 3 years ago

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.

sindresorhus commented 3 years ago

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.

fregante commented 1 year ago

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.

sindresorhus commented 1 year ago

Dependencies are no longer lazy loaded.

sindresorhus commented 1 year ago

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