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

Support cross platform environment variables #127

Open
backspaces opened this issue Feb 6, 2018 · 18 comments
Open

Support cross platform environment variables #127

backspaces opened this issue Feb 6, 2018 · 18 comments

Comments

@backspaces
Copy link

Add a cross-env feature which resolves environment variables in a standard way.

Example: I like to use env vars so that configuration information can be stored in package.json. I believe the following will fail on windows but works fine on mac:

  "mkdirs": "dist docs/dist docs/models",
  "scripts": {
    "clean": "shx rm -rf $npm_package_mkdirs && shx mkdir $npm_package_mkdirs",
@backspaces
Copy link
Author

BTW: This works, but is pretty ugly:

"clean": "cross-env-shell \"shx rm -rf $npm_package_mkdirs && shx mkdir $npm_package_mkdirs\"",

.. but I don't have a windows environment to check this works there. It may simply work on mac/linux anyway.

@nfischer
Copy link
Member

nfischer commented Feb 7, 2018

Just to clarify, npm_package_mkdirs is an environmental variable?


#65 mentions the cross-env package too.

This sounds like a reasonable feature for shx (shelljs already exposes this under shell.env.NAME_OF_VAR). In general, shell variables can be quite complex due to string manipulation (I implemented a subset of bash-isms at nfischer/shelljs-transpiler#5).

If we want to support this, we need to decide how far we should go:

  • Basic support: $FOO ➡️ shell.env.FOO
  • Brackets: ${FOO} ➡️ shell.env.FOO
  • Prefix/suffix: prefix${FOO}suffix ➡️ 'prefix' + shell.env.FOO + 'suffix'
  • And so many more edge cases...

@backspaces what do you think fits the needs for most users?

@backspaces
Copy link
Author

Yes, the code above is part of my package.json which I used to store configuration information for scripts. Looks like this:

  "mkdirs": "dist docs/dist docs/models",
  "scripts": {
    "clean": "cross-env-shell \"shx rm -rf $npm_package_mkdirs && shx mkdir $npm_package_mkdirs\"",
    "build": "npm-run-all clean build-libs build-dist minify build-docs",
    "build-libs": "node bin/wraplibs.js",
    "build-dist": "rollup -c",
    "build-docs": "shx cp -R README.md dist docs && node bin/docsmodels.js",
    "minify": "squash dist/AS.js > dist/AS.min.js && squash dist/AS.module.js > dist/AS.module.min.js",
    "test": "ava --verbose"
  },

So in my use-case, I would like

shx rm -rf $npm_package_mkdirs

To be expanded to

shx rm -rf dist docs/dist docs/models

I think that would be the basic support case.

I'm not sure if you'd need a way to set env vars. I haven't run across that.

@nfischer
Copy link
Member

nfischer commented Feb 7, 2018

Oh, I see. I wasn't familiar that npm package attributes were set as environmental variables. Thanks for explaining.

Another thing we have to worry about: $npm_package_mkdirs could contain spaces, and POSIX shells would expand that into multiple words. We're a bit constrained here. We have no way to properly distinguish when the user passes quotes:

shx rm -rf $npm_package_mkdirs
# This is an 'rm' with 3 directory arguments

shx rm -rf "$npm_package_mkdirs"
# This has 1 directory argument (contains spaces in the name)

Would it be reasonable to support only single-word package variables?

@backspaces
Copy link
Author

backspaces commented Feb 7, 2018

Would it be reasonable to support only single-word package variables?

I'm not sure what you mean by that. If it means my 3 dir example fails, then nope! :)

But seriously, I believe the usual expansion of $foo is simply the value, spaces and all, of the $foo env var. Your second examples is fine for when the spaces are really part of the file name.

As for spaces in the dir names, couldn't the programmer simply quote them like this:
export foo='1 "2 3"' which produces: echo $foo => 1 "2 3"

Note that env vars can come from lots of different files, not just package.json. AND they needn't be env vars but simply vars like so in npm scripts:

   "foo": "files='one \"two three\"' && shx echo $files"

This is likely not cross-platform, however. But wouldn't shx export files='one \"two three\"' be cool!

Maybe the issue is node support. I found this:
https://www.twilio.com/blog/2017/08/working-with-environment-variables-in-node-js.html
.. mainly to emphasize their popularity.

@backspaces
Copy link
Author

To simplify:

@nfischer
Copy link
Member

nfischer commented Feb 8, 2018

As for spaces in the dir names, couldn't the programmer simply quote them like this:
export foo='1 "2 3"' which produces: echo $foo => 1 "2 3"

Try this:

$ export foo='first_file.txt "second file with spaces.txt"'
$ for file in $foo; do echo $file; done
first_file.txt
"second
file
with
spaces.txt"
$ # Yikes! I was hoping for 1 file per line

Side note: this is why bash has arrays. But you can't put arrays in env vars, so that isn't useful for shx.

Note that env vars can come from lots of different files, not just package.json. AND they needn't be env vars but simply vars like so in npm scripts:

Non-env vars are infeasible: these are scoped to the current shell, so shx (which runs as a subprocess) can't look-up their value.

Maybe the issue is node support

The issue is that npm runs scripts in a shell (/bin/sh or cmd.exe). Shells manipulate CLI arguments before launching the sub process (in our case, that's shx). Imagine we've set a variable like so:

$ export foo='bar     baz'

Consider how this looks to the shx process:

shx command desired output (unix) process.argv.slice(3) on unix process.argv.slice(3) on Windows
shx echo $foo bar baz [ 'bar', 'baz'] ['$foo']
shx echo "$foo" bar·····baz [ 'bar·····baz'] ['$foo']
shx echo '$foo' $foo [ '$foo'] I forget exactly, either ['$foo'] or ["'$foo'"]

(Pretend that the · characters are spaces, markdown rendering squashes consecutive whitespace)

Our first big problem is that every case looks the same on Windows. If we can't distinguish, then which behavior do we choose? If we assume any of the cases, then we've broken the others.

@backspaces
Copy link
Author

Sigh. Brilliant explanation, thank you.

My current solution uses cross-env-shell, in the second post above. I wonder how they handle it? We could check their code.

But I believe their problem is different: they mainly convert between $foo and %foo% formats and they let the unix/windows shells handle it.

Historically: I got here by initially using bash backticks to create arguments for commands:

"foo": "rm -rf `pkgkey.js('mkdirs')`

where pkgkey simply reads in package.json, converts it to a JS object and returns the key's value as a string. It's a hack, but see below.

However, I soon found that backticks don't work in windows. So finally discovered env vars as a solution. I don't suppose backticks make sense for shx?

Here's pkgkey.js:

#!/usr/bin/env node
const fs = require('fs')
const json = JSON.parse(fs.readFileSync('package.json'))

const key = process.argv[2]
if (!key) throw new Error('pkgkey called with no argument')
let val = json[key]
if (!val) throw new Error(`pkgkey: ${key} not found`)

if (Array.isArray(val)) val = val.join(' ')

process.stdout.write(val)

@backspaces
Copy link
Author

Hmm. Just a thought: would backticks be easier to implement than env vars?

@nfischer
Copy link
Member

nfischer commented Feb 8, 2018

My current solution uses cross-env-shell, in the second post above. I wonder how they handle it? We could check their code.

They're accepting a full command string. So process.argv looks more like ['shx cp $files "$dest"'] instead of ['cp', '$files', '$dest']. The tradeoff is that they need to do all the parsing that a shell would normally do, but they get to see exactly what the user typed.

However, I soon found that backticks don't work in windows. So finally discovered env vars as a solution. I don't suppose backticks make sense for shx?

Not exactly our purpose. We're trying to expose shelljs command implementations to the CLI, not execute arbitrary commands.


If you're not opposed to writing a script, how about using shelljs directly?

// package.json
{
...
  "mkdirs": [ "first dir", "second dir", "third dir" ],
...
}
// clean.js (hook this up to `npm run clean`)
var shell = require('shelljs');

var dirList = require('./package.json').mkdirs;

var ret = shell.rm('-rf', ...dirList);
if (ret.code === 0) shell.mkdir(...dirList);

@backspaces
Copy link
Author

Nice approach! I may do that.

I use shelljs in nearly all my node utilities. Like the es6 usage, thanks. And didn't know I could require a json file, that is cool.

@nfischer
Copy link
Member

nfischer commented Feb 9, 2018

Yeah, unfortunately it seems like there's no obvious way to support env variables without running into these edge cases. It's a good feature request, and it would've been really nice if we had a path forward.

For tough cases (and this appears to be one) we still need to resort back to ShellJS. While a short script isn't as elegant as a shell one-liner, it offers more flexibility and power.

@itaysabato
Copy link

@nfischer I would guess upwards of 90% of use-cases are super simple single word variables, e.g. $OPTION that would resolve to true or false or some path without spaces.

Can we not start with this very simple support and work our way up to more complex edge cases as we go? The limitations can be documented and the edge-cases can be addressed incrementally.

I think this is a great tool and it certainly made my life easier, but, for me, this one small thing would make a lot of difference.

@nfischer
Copy link
Member

Can we not start with this very simple support and work our way up to more complex edge cases as we go?

The issue is that supporting that case would break other cases. Basically, cp '$foo.txt' would behave differently than shx cp '$foo.txt', which may surprise users.

The limitations can be documented and the edge-cases can be addressed incrementally.

Yes, we can document our behavior. But just to reiterate, we cannot address edge cases incrementally: we face fundamental limitations which are impossible to resolve.

If we were to implement this, the best case we could achieve is:

  • only support exported environmental variables
  • always assume $foo means "expand $foo to its value"
    • breaks cases like shx echo 'Print literal $foo' (could be controlled by a switch)
  • expand $foo as if it were double-quoted (expand it to one string, preserving white space)
    • doesn't satisfy the original use case for this bug report (could be controlled by a switch)
  • too difficult to support all the bashisms for manipulating strings (e.g. ${foo}, ${#foo}, ${foo/a/b}, etc.)
    • possible to implement a subset, but not reasonable to get everything
  • can't distinguish cases like "$foo"bar and "$foobar"

Basically, we have to assume what the user intends, which means sometimes we'll assume incorrectly. We could provide switches to assume the opposite way, but I'm not promising these would be good ideas or reasonable to support.

@backspaces
Copy link
Author

backspaces commented Mar 2, 2018

@nfischer: btw, the example you show of for file in $foo; do echo $file; done is the bane of bash programmers. I.e. spaces in any name is a nightmare. Even for bash.

How do bash programmers solve it? A common solution is read

http://wiki.bash-hackers.org/commands/builtin/read

I know I typically do that:

echo 'a
b c' | while read file; do echo $file; done

Produces:

a
b c

For files, consider this directory, bar with two files "a" and "b c"

ls -1
a
b c

And

ls -1 | while read file; do echo $file; done

.. handles file names with spaces.

I think shx's ls does fine:
shx ls
a
b c

So basically, I'd say env vars with spaces are the user's problem, there are MANY way to handle them, just don't use spaces for both separators and in file names. So export foo="a:b c" would use : for separator and spaces in file names.

Here's how to say what $foo does in the shx/shelljs world: exactly the same as process.env.foo does. Period. If I need to deal with edge cases, that's my concern. Just like with bash! : )

@frastlin
Copy link

frastlin commented Dec 1, 2019

Can we just start with node_env? Pretty much every single user of this package will be using node_env sometime in their scripts, and it's a pain that on OSX and Linux it's 'node_env="production"' and on Windows it's 'set node_env="production"'.
cross-env solves this problem, but I would like a simple solution in shx.
Another option is if there are if statements that can run in shx, and you can do one for windows and the other for everything else.

@nfischer nfischer added the feat label Oct 30, 2020
@goerwin
Copy link

goerwin commented Feb 3, 2021

What about setting the env vars in the same line, without having to restort to cross-env? Eg:

"scripts": {
    "build:dev": "shx NODE_ENV=development webpack"
    "build": "shx NODE_ENV=production webpack",
    ....

@nfischer
Copy link
Member

nfischer commented Feb 5, 2021

@goerwin we don't support executing arbitrary commands at the moment, so I don't think there's any benefit to defining variables in-line (although I can see the benefit of combining these features together). Other than that, this is subject to the same concerns I outlined above (distinguishing between $foo vs. '$foo', VAR=value vs. 'VAR=value', etc.).

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

No branches or pull requests

5 participants