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

remove 'nice-try' dependency #102

Conversation

andrusieczko
Copy link

node-cross-spawn is a dependency of 2431 packages, including highly popular packages eslint or webpack-cli.

This means that the package itself should reduce its own dependencies to the bare minimum.

The nice-try package does nothing but swallowing the error of the invoked function.
It should never be considered as an external dependency - it is too brittle.
Now, nice-try found its way to be included in most of the projects that use eslint or webpack which is totally unnecessary.

Hence, I propose to remove the dependency.

@satazor
Copy link
Contributor

satazor commented Jul 28, 2018

Hello!

The dependency is small and yes, the installation might be a little bit slower (~100ms slower?). I see a lot of developers doing what you just did but it goes against my ideals of code-reuse and dependency management.

Thanks for the PR though.

@satazor satazor closed this Jul 28, 2018
@andrusieczko
Copy link
Author

andrusieczko commented Jul 28, 2018

Hi @satazor,

I recommend reading about left-pad fiasco:
https://www.haneycodes.net/npm-left-pad-have-we-forgotten-how-to-program/
https://www.theregister.co.uk/2016/03/23/npm_left_pad_chaos/
https://blog.npmjs.org/post/141577284765/kik-left-pad-and-npm

I'm all for code usability whenever it makes sense.

I just don't this this qualifies for this :)

@satazor
Copy link
Contributor

satazor commented Jul 28, 2018

I have already read it. I’m sorry but I don’t really have time do discuss this subject nor I have the energy.

@andrusieczko
Copy link
Author

I very much appreciate all the work you put in the package, you really deserve that.

The company I work for relies on your code and I spotted this unnecessary dependency that must sit in our system.
So I thought I'd come and help remove it.

Not sure where is this "I don't want to discuss this" coming from... but it's your package and it's totally up to you. I just came with a friendly advice.
If you don't like it - well, I have no other choice than just shut up and leave ;)

@satazor
Copy link
Contributor

satazor commented Jul 28, 2018

I didn't want to be rude but I was being sincere. The whole subject is controversial and, in the end, it's a personal preference and developers' ideals.

I'm in a tight timeline and I really need all the time I can have. Having the said, I don't have time to write a detailed answer of why I think it's wrong to "inline" dependencies.

Again, thanks for taking the time to do this PR and trying to engage in a conversation where you justified your arguments. But, unfortunately, I don't have the time to justify mine.

@vmcall
Copy link

vmcall commented Oct 12, 2018

If you're so busy that you can't explain your decision to not inline a single function, why are you writing over 100 words to explain how busy you are?

@phase
Copy link

phase commented Oct 12, 2018

it goes against my ideals of code-reuse and dependency management

This isn't code reuse, this is code fragmentation. One line functions don't help you reuse anything. The function itself is an awful idea, as you should be handling errors instead of throwing them away, but putting in another dependency for a one line function isn't reusing code.

@ghost
Copy link

ghost commented Oct 12, 2018

Informative

@moxystudio moxystudio locked as too heated and limited conversation to collaborators Oct 12, 2018
@satazor
Copy link
Contributor

satazor commented Oct 12, 2018

Besides not agreeing with "inlining" the dependency, this PR delivers low impact changes.

This has escalated quickly and, as such, I'm going to lock down the thread.

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

Successfully merging this pull request may close these issues.

4 participants