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

Matches babel env preset instead of engine #31

Closed
haysmike opened this issue May 16, 2018 · 7 comments
Closed

Matches babel env preset instead of engine #31

haysmike opened this issue May 16, 2018 · 7 comments

Comments

@haysmike
Copy link
Contributor

Hello! I've found a bug. The regex here also matches Babel preset options. I happen to be using the "node": "current" Babel env setting for a few projects, which causes the plugin to report package-json-engine: no version satisfying `current' installed. Let me know if you're open to PRs, thanks!

@jasonkarns
Copy link
Member

Yes, thank you! Though I am not excited to iterate on tests of the regex ;)

@haysmike
Copy link
Contributor Author

Are you open to pulling in jq as a dependency? The method in question could just do jq .engines.node $package_json_path

@jasonkarns
Copy link
Member

Probably not, because there really isn't a way for this project to provide deps. Which means it would be considered an external dep that a user must install/configure on their own, instead of being self-contained (or installed automatically).

Consider the difference with the sh-semver script that is bundled within this project such that git-cloning the plugin within nodenv plugins just works.

We might consider it, but only as a last resort.

@jasonkarns
Copy link
Member

jasonkarns commented May 16, 2018

On the flip side, I don't think that leveraging node itself would be terrible. We could almost assume a node existing in PATH somewhere. So perhaps porting the logic as a node script would solve the complexity, without adding a dep that isn't reasonably expected to be available. (Provided the node script only uses core apis and doesn't use any npm deps.)

@hurrymaplelad
Copy link
Contributor

Thanks for the heads up! How 'bout JSON.sh, committed as node_module as we do with sh-semver?

PRs welcome!

@jasonkarns
Copy link
Member

Oh nice! And JSON.sh looks fairly small, too. I'm 👍 on using it and vendoring as sh-semver.

(Though separately, I bet JSON.sh could use a PR that adds files to its package.json. They're including an awful lot of irrelevant files in their npm tarball.)

@haysmike
Copy link
Contributor Author

Thanks for the guidance, I'll look into JSON.sh when I get a chance!

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

No branches or pull requests

3 participants