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

Add execjs to dependencies #269

Closed

Conversation

shuheiktgw
Copy link

This PR relates to #234

When I run pre-commit, it warns me Could not load execjs: cannot load such file -- execjs. I found In js plugin, pre-commit checks if it can load execjs or not. I think it is kind of inconvenient I need to manually install execjs in order to use js plugin, so added it to dependency! 👍

@jish
Copy link
Owner

jish commented Jun 17, 2018

Hmm... let me think about this. I'm not sure if the execjs dependency is missing on purpose or by accident. 🤔

On the one hand we want to ship a working program out of the box, so execjs should be an explicit dependency.

But on the other hand, I believe there are some projects that want to put pre-commit in their Gemfile and also do not want execjs as part of their bundle.

@shajith do you have an opinion on this?

@shuheiktgw
Copy link
Author

Oh, now I see the intention of this code! 👍
Hmm, what if I discard this change, and just make the warning message more developer friendly to tell the developers that they need to add execjs to their dependency manually?

@jish
Copy link
Owner

jish commented Jun 18, 2018

"Oh, now I see the intention of this code! 👍"
"Hmm, what if I discard this change, and just make the warning message more developer friendly to tell the developers that they need to add execjs to their dependency manually?"

👍 That would be great. A better message would be helpful no matter what path we choose going forward 🙂

@shuheiktgw
Copy link
Author

Thank you for your comment!
I just made another PR #273!

@shuheiktgw shuheiktgw closed this Jun 19, 2018
@shuheiktgw shuheiktgw deleted the add_execjs_to_dependencies branch June 19, 2018 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants