sanack / node-jq

Node.js wrapper for jq
MIT License
276 stars 51 forks source link

Add custom args to options #655

Closed hirenr closed 7 months ago

hirenr commented 7 months ago

node-jq is an excellent library. However it is a bit limited by functionality. I added the capability to send custom args to the jq command to fully unlock the functionality and make it available via this library.

davesnx commented 7 months ago

I'm not against this change, but I would like to know what args are you missing from node-jq than you need this, rather than opening the door for all arguments, since it won't solve the issue of what happens to users when passing opposite values on args and our options object.

hirenr commented 7 months ago

So, I am currently building a node based runtime for Serverlessworkflow.io which basically uses jq syntax as part of the specification. To confirm to the spec requirement (passing context object when running jq),

I needed the use of the following commands so that I am able to have flexibility in passing objects.

--arg name value      set $name to the string value;
 --argjson name value  set $name to the JSON value;
--slurpfile name file set $name to an array of JSON values read from the file;
--rawfile name file   set $name to string contents of file;
--args                consume remaining arguments as positional string values;
--jsonargs            consume remaining arguments as positional JSON values;

I do not believe there is a conflict, since jq command always takes the last value as presented in the command. Anything specified in the advanced args object will always override the other arguments if there is a conflict.

davesnx commented 7 months ago

I still would prefer to add support for all those flags, rather than add a generic "args". Would you be up for adding those as a PR instead?

hirenr commented 7 months ago

I tried doing that, but could not figure out the specific requirements of picking the next two args when detecting a flag etc. eg. --arg name value Could not work It out with joi.

My 2cents... a wrapper library should not act as a gatekeeper but as a facilitator. While the specific flags are useful and makes life easy and should be implemented, but for advanced and complex use cases the generic args option is useful. Just so that we do not block any use case.

I will dig deeper to understand Joi better and create PR's to implement these flags, but in the meantime, I do urge that we merge a version of this. I really do not want to create/maintain a fork and I also do not want to implement a way to call binaries directly. This library just works best.

hirenr commented 7 months ago

Refer to this example. FFmpeg is a popular nodejs wrapper over FFmpeg binary and is used to convert media.

It includes a custom option to expose/passthrough args to allow features not exposed by the wrapper.

This feature is useful and can be seen as a bridge/short term solution until the wrapper can include the right options.

davesnx commented 7 months ago

Appreciate the feedback but indeed the wrapper library isn't a gatekeeper, it tries to expose the same API as a JS API. If we allow an any object is losing the purpose of the library.

I would encourage you to leave the joi part if you can and I might be able to make it work.

I'm not trying to say no to this PR, but rather understand your use-case and bring a good experience to every user (in the future)

hirenr commented 7 months ago

Ok. so I have removed the ability to add any custom commands. The args option now takes key value arguments and translate it to either --arg or --argjson depending on the type of value.

WDYT? These are the two commands I need right now. Once I feel the need for the others, I will create more PR's to add them.

hirenr commented 7 months ago

Done!

hirenr commented 7 months ago

quick qn... i just saw the release action. looks like the commit did not trigger a release to npm! did i miss anything.

github-actions[bot] commented 7 months ago

:tada: This PR is included in version 4.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: