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

Reduce dependencies where prudent #6658

Closed
brendankenny opened this issue Nov 27, 2018 · 10 comments
Closed

Reduce dependencies where prudent #6658

brendankenny opened this issue Nov 27, 2018 · 10 comments
Labels

Comments

@brendankenny
Copy link
Member

brendankenny commented Nov 27, 2018

It's been a while since we've audited these :)

screen shot 2018-11-26 at 16 18 51

https://npm.anvaka.com/#/view/2d/lighthouse

Our biggest win would be to get rid of update-notifier. It brings in 52 indirect dependencies. Yarn just does this check themselves, or we could just not do it. Removing update-notifier would bring us from 174 deps to 122.

Next would be inquirer, which we use just for the prompt on if we can use Sentry on first run of the CLI. Not sure what happened in that community, but there's basically a reimplementation with an almost exact API match in enquirer, and it only has one dep. This would bring us from 122 deps down to 92.

Replacing yargs with commander would remove 15 dependencies.

Moving chrome-launcher's @types deps to dependency deps would remove 4 dependencies

After that comes

Though I'm not sure if we can do much about configstore and raven, and the competitors to rimraf tend to also use its only dependency, glob, so have the same or more dependencies.

@patrickhulce
Copy link
Collaborator

honestly we could also make rimraf an optional dep, it's exclusively used for -G which I doubt many folks use that :)

That being said, what is the primary motivation for this? All of these only affect code size of the CLI and we don't update them ever.

@connorjclark
Copy link
Collaborator

I assume it's motivated by the recent malicious takeover of the popular package event-stream. A nice reminder to keep dependencies to a minimum.

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 27, 2018

Could we replace rimraf entirely? I don't think the -G feature requires the full power of globs. We could have a small rm -rf latest-run/ function (or rd /s /q "latest-run/" in windows... according to stackoverflow). I assume that's all we need.

@patrickhulce
Copy link
Collaborator

patrickhulce commented Nov 27, 2018

I assume it's motivated by the recent malicious takeover of the popular package event-stream. A nice reminder to keep dependencies to a minimum.

Sure, but if we're not updating them and we're not currently exposed, then it seems like a lot of work for no immediate need. If this is our concern, I think it might make more sense to institute some harsher check on bringing in/updating dependencies rather than going to remove our existing ones before we need to for example.

EDIT: Or pinning our dependencies more strictly, etc

@brendankenny brendankenny changed the title Reduce dependencies Reduce dependencies where prudent Nov 27, 2018
@brendankenny
Copy link
Member Author

That being said, what is the primary motivation for this? All of these only affect code size of the CLI and we don't update them ever.

Sorry, should have put more of a scope on this.

More dependencies aren't necessarily a negative, but they definitely aren't a positive. If we can reduce the number of dependencies while maintaining the features we want and not creating pain in development or maintenance, that's a good thing.

inquirer might have been a better first entry in the initial comment. A near drop-in replacement exists that would provide a ~30 dep/~24MB reduction after npm i. They also claim a ~250ms reduction in startup time (on some machine) due to the large number of requires needed to load it. That's a lot of downside to the current solution for a single confirm screen seen at most once.

Testing it out, the main thing that we'd need to fix to match master would be manually bolding the question text and overriding their ctrl-c handler (which is stomping on ours in the timeout case).

I think arguably update-notifier is in a similar place with 52 deps independent of any of our other ones. It's not a lot of functionality for a good bit of cost. It's less clear if there's an easy replacement or if we'd want to maintain something ourselves (we already have configstore and semver, so we're like 2/3 of the way there :), but it's worth weighing at least.

@patrickhulce
Copy link
Collaborator

Gotcha! Scope makes more sense now thanks! :)

@jonschlinkert
Copy link

Testing it out, the main thing that we'd need to fix to match master would be manually bolding the question text and overriding their ctrl-c handler (which is stomping on ours in the timeout case).

@brendankenny I maintain enquirer and would be happy to help or answer questions. FWIW, both of the things you mentioned should be easy to do with Enquirer. We should be able to override the ctrl-c handler by setting a custom action name on the actions option ("actions" are just mappings from keypresses to method names). The question text is also easy to customize. It should already be bold by default, but I quite possibly missed it somewhere.

@jonschlinkert
Copy link

Just noticed the configstore comment. We use data-store quite a bit, I'm not sure how different the APIs are, it's been a while since I looked at configstore, but if I recall correctly both libs have a pretty small API surface.

@wardpeet
Copy link
Collaborator

wardpeet commented Dec 2, 2018

Could we replace rimraf entirely? I don't think the -G feature requires the full power of globs. We could have a small rm -rf latest-run/ function (or rd /s /q "latest-run/" in windows... according to stackoverflow). I assume that's all we need.

I wouldn't remove rimraf, it has some nifty retry mechanism for windows... If there is another on that does the same, feel free to switch.

@brendankenny
Copy link
Member Author

this is also an issue that could live eternally. Let's close and going forward I'll try to focus my unease on specific ideas :)

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

No branches or pull requests

6 participants