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

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

Open
fregante opened this issue Jul 26, 2021 · 4 comments
Open

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

fregante opened this issue Jul 26, 2021 · 4 comments

Comments

@fregante
Copy link
Contributor

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.
#82 (comment)

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:

@fregante fregante changed the title Run in child process instead of lazy loading Run in child process instead of lazy loading every dependency Jul 26, 2021
@sindresorhus
Copy link
Owner

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

fregante commented Feb 9, 2023

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
Copy link
Owner

Dependencies are no longer lazy loaded.

@sindresorhus
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants