Specify an encoding?
rauschma opened this issue · 10 comments
I love the idea behind this library!
How about implementing the Proxy like this:
const {spawnSync} = require('child_process');
module.exports = new Proxy(
{},
{
get: (_, bin) => (...args) => {
return spawnSync(bin, args, {encoding: 'utf-8'});
},
}
)
A benefit of specifying an encoding is that you don’t need this code anymore: Object.assign(new String(out.stdout), out)
Are you using {...this}
so that users can change the options of spawnSync()
? Then I’d prefer a solution that doesn’t mutate a global object – maybe:
function createShell(userOptions = {}) {
const options = {encoding: 'utf-8', ...userOptions};
return new Proxy(
{},
{
get: (_, bin) => (...args) => {
return spawnSync(bin, args, {encoding: 'utf-8'});
},
}
);
}
const sh = createShell();
console.log(sh.ls('-la'));
A benefit of specifying an encoding is that you don’t need this code anymore: Object.assign(new String(out.stdout), out)
This is to be able to do two this: git().status
and git().trim()
for example.
Are you using {...this} so that users can change the options of spawnSync()?
Yes) It's kinda a hacky way. As bin is passed as func name, args as args so we are left only with this context.
git.call({encoding: 'utf-8'}, 'status')
Yes. It's kinda a hacky way.
Ah, clever! (I overlooked that in the readme.)
I thought you wanted people to add properties to the Proxy, but specifying this
via .call()
is a clean solution.
I’d still use 'utf-8'
as (overridable) default then:
const out = require('child_process').spawnSync(bin, args, {encoding: 'utf-8', ...this})
This is to be able to do two this:
git().status
andgit().trim()
for example.
Personally, I’d prefer simply returning out
:
- Then the first expression doesn’t change.
- The second expression becomes
git().stdout.trim()
.
But I can see why you have chosen a different approach.
This boxed string allows, also, to do this:
let out = cat('file.txt')
console.log(`Content: ${out}`)
encoding: 'utf-8'
We can add it as default for sure.
If I may – I have one more question about this line:
const out = require('child_process').spawnSync(bin, args, {...this})
AFAICT, strict mode should be enabled in this module because {...this}
will spread the global object if the surrounding function is function-invoked (i.e., not via .call()
).
Without Object.assign()
, the code would be:
let out = cat('file.txt')
console.log(`Content: ${out.stdout}`)
I’d be OK with that, but it is more verbose.
Oh. Yea. This should be fixed.
I’ve used tinysh some more and I now agree with you that returning strings (or something string-like) is convenient. It therefore may make sense to switch from spawnSync()
to execFileSync()
:
- It delivers all failures (child process can’t be spawned, shell error, process killed) via exceptions. In contrast,
spawnSync()
only throws if spawning fails. - It returns a string with the content of stdio (if the encoding is
'utf-8'
).- Given that all failures are handled via exceptions, returning stdio strings might be enough – we might not need the other values returned by
spawnSync()
.
- Given that all failures are handled via exceptions, returning stdio strings might be enough – we might not need the other values returned by
Either with execFileSync()
or with spawnSync()
, I’d set options.shell
to true
:
- You basically can’t do anything on Windows if it doesn’t have that value.
- On Unix, it’s nice if wildcards work:
ls('-l', '*.txt')
In other words: I’ve used the following Proxy in my experiments and I liked how it worked:
new Proxy({}, {
get: (_, bin) => function (...args) {
return execFileSync(bin, args,
{
encoding: 'utf-8',
shell: true,
...this
}
);
},
})
BTW, have you seen my other library with similar goal? https://github.com/google/zx
Ah, cool, I wasn’t aware you wrote zx
, too! I’ve been aware of that library for a while. Quite useful.