Lazy load dependencies

[nikhilkh]

No description provided.

[nikhilkh]

Alternative approach to #81. Fixes #80

[SBoudrias]

Really not a fan of lazy loading dependencies... cc @sindresorhus

[nexdrew]

@nikhilkh Unfortunately this will need to be rebased.

@SBoudrias I'm usually not a fan of lazy loading deps either, but, due to the nature of this module ("extra" functionality that is non-critical to a CLI, goal of being non-intrusive), I think it's not a bad idea. Hey, at least he chose Sindre's lazy-req for the implementation. 😃

[nexdrew]

If we do eventually merge this, we should put this dependency in alphabetical order. Typically npm will do this for us via npm install --save lazy-req.

[nikhilkh]

I've updated the PR. I wouldn't necessarily lean on lazy loading either but for a module that runs on startup to provide extra features for a CLI - it's important to have minimal impact on startup perf. #80 describes the impact in the current state. Loading latest-version, chalk etc and their dependencies add undue startup time overhead without any benefit. #80 shows the perf impact on a warm scenario. The impact is even more significant in a cold load scenario,

[sindresorhus]

Not a big fan either @SBoudrias, 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.

[sindresorhus]

Changes looks good to me, but should be manually tested before doing a release to ensure it didn't break anything.

[nexdrew]

@sindresorhus Worth adding a test that verifies an update-notifier dependency does not show up in the require.cache until after some logic has run? Or is that just a duplication of lazy-req testing?

[sindresorhus]

@nexdrew Yeah, I don't think that is of much help. What's needed here in general are more integration tests, testing that the notification actually shows up when there's a new release. Right now, I manually delete the update-notifier configstore entry for yo (since it depends on update-notifier), and run it with a fake lower version, so it will trigger update-notifier.

[nexdrew]

@sindresorhus Ok, gotcha. That's what I do when manually testing as well.

[sindresorhus]

Opened an issue: #88

[SBoudrias]

@nikhilkh hey, any chance you can fix the merge conflict? Given the other maintainers are fine with going the lazy-req road, I think this would be mergeable as is.

[nikhilkh]

Fixed merge conflict. Thanks! It will be great to have this merged soon to avoid future re-basing. :)

[SBoudrias]

Thanks!