New: `nodepath` option for the path by gyandeeps · Pull Request #534 · shelljs/shelljs

gyandeeps · 2016-10-20T02:13:29Z

This what i am thinking for nodepath config option. This works as i have tried it.
Its getting difficult to test since when you run npm test to run the unit test. NPM already adds the ./node_modules/.bin to the $PATH.

I will work on the tests but I wanted to know whether I am heading in the right direction.

nfischer · 2016-10-20T03:23:12Z

src/common.js

 var shellMethods = Object.create(shell);
+var PATH = 'PATH';
+
+// windows calls it's path 'Path' usually, but this is not guaranteed.


I believe we can rely on process.env to have case-insensitive keys, so process.env.PATH should always work.

process.env isn't a real JS object. See atom/atom#11302 as an example of this.

I saw npm doing this that why I did it to make sure we are cross platform

Hmmm could you link me to the line in npm? I know that process.env is case-insensitive on my Windows machine. Perhaps it's not the case for everyone?

I would imagine things would get very broken in NodeJS if process.env were case-sensitive on Windows. Perhaps that's the case for older versions of Node?

https://github.com/npm/npm/blob/master/lib/utils/lifecycle.js#L20-L28

thumbsup means keep the change or remove it? because this change is tied with the below function which we are talking abt also.

I posted a follow up comment. It looks to me like they're making a PR to add this to the documentation (I didn't read the PR yet, so I could be mistaken). This seems to indicate that this is the intended behavior on Node on Windows.

My guess is that process.env has always had case-insensitive keys on Windows, but it was just never documented. I think it's ok to rely on that (unless we hear otherwise).

@nfischer let me know whatever you thoughts are on this and I will react accordingly. Thanks

@gyandeeps let's go for the simpler approach, process.env.PATH. Thanks

Apparently this is not true. I use windows 10 and when I use windows default cmd then its called Path but if I run using git bash then its called PATH.
Thats why I see test fail on windows.

@gyandeeps if this is not the case, then it would be good to chime in on nodejs/node#9157 and the corresponding pull request before Node 7 gets incorrect documentation.

As for this PR, let's go with your initial approach. Sorry for the confusion.

nfischer · 2016-10-20T03:24:15Z

src/common.js

  globOptions: {},
-  maxdepth: 255
+  maxdepth: 255,
+  nodepath: false


Could you add a trailing comma? We don't enforce it via linting, but it's something we're trying to move toward to keep diffs simple.

nfischer · 2016-10-20T03:33:57Z

src/common.js

+  var env = objectAssign({}, process.env);
+
+  if (config.nodepath) {
+    env[PATH] += (process.platform === 'win32' ? ';' : ':') + path.join(process.cwd(), 'node_modules', '.bin') + (process.platform === 'win32' ? ';' : ':');


Could you change this to path.delimiter?

Also, I don't think we always want a trailing delimiter, so you can get rid of the second (process.platform === 'win32' ? ';' : ':'). Please correct me if Windows differs from Unix on this point.

nfischer · 2016-10-20T03:34:26Z

src/common.js

+  return env;
+};
+
+exports.getPathFromEnv = function (env) {


I don't think we need this, since we can just use process.env.PATH to retrieve the value.

At some places in the code base we have used process.env.path || process.env.Path || process.env.PATH; to get the correct path. I just made a function to keep it at one spot.

Yeah, I saw that. I think that wasn't actually necessary for us to do. We should probably convert everything to process.env.PATH for the sake of consistency.

Plus, we would run into issues if someone on unix ever did export path=foo (totally valid for a user to do, but it would provide incorrect results for which()).

I am hearing get rid of this change and the above logic and just make it use process.env.PATH, correct?
Will make the change then.

Yeah. Let's try that. If things break on Appveyor, then we can revert back to this.

nfischer · 2016-10-24T06:09:55Z

Let's add a test in exec() and in which()

gyandeeps · 2016-10-25T04:18:56Z

@nfischer All the work has been done. Some of things to note:

Apparently PATH is not same across. I use windows 10 and when I use windows default cmd then its called Path but if I run using git bash then its called PATH. Thats why I see test fail on windows.
I have added sinon so that we can fake process.env.PATH for testing purposes.
I think we should soon move to using some kind of testing framework since unit test are not well managed currently.

nfischer · 2016-10-25T05:20:50Z

@gyandeeps

See my comment in-line
sinon is fine
Please see test: switch to AVA #512. I'll try to rebase off master sometime this week. There's still quite a bit of work to do, however. If you're interested in helping, feel free to make a PR against that branch.

gyandeeps · 2016-10-25T13:07:09Z

Thanks @nfischer . I think PR is ready. Let me know if you have any suggestions.

nfischer · 2016-10-25T21:40:58Z

@gyandeeps I'll take a look later today. If you can, please chime in on nodejs/node#9157, since that just added got merged into official node documentation, and it disagrees with your observations on process.env case-sensitivity. It would be bad if the documentation was incorrect but got merged anyway.

nfischer · 2016-11-03T05:25:20Z

@gyandeeps Sorry for the late reply. I got a bit distracted with #512

Here's my thoughts on this PR:

shell.env (links to process.env) is already part of our API, so I'm concerned about moving away from that to a custom function. It feels a little clunky to use, especially with common.PATH_IDENTIFIER. I wanted to get your feedback on a slightly different approach.

Instead of having code to get process.env.PATH (regardless of how PATH is capitalized), what if we just use process.env.PATH = process.env.PATH || process.env.Path || process.env.path? Then we don't need a function or PATH_IDENTIFIER, we can just use process.env.PATH.

We can write a setter for config.nodepath to add (or remove) node_modules/.bin to the PATH.

Let me know what your thoughts are for this approach. The downside is that this modifies a global, but the option is likely not going to be used often, and I can't think of a use for this option where modifying the PATH that way would be dangerous.

Although admittedly, that approach wouldn't be anything the users wouldn't be able to do themselves, so perhaps it's best to leave the code as-is.

nfischer · 2016-11-03T05:01:12Z

src/common.js

+
+// windows calls it's path 'Path' usually, but this is not guaranteed.
+if (process.platform === 'win32') {
+  PATH_IDENTIFIER = 'Path';


Is this line necessary? It'll get overridden in the loop below.

gyandeeps · 2016-11-03T14:29:51Z

My thoughts:

We can definitely change the PATH approach and do what ever you said.
Why I create a copy of path is because I dont want to change the path inside the whole nodejs process. Since its single process, me changing the path would/might impact other things in the environment. That was my thought process when designing this.

nfischer · 2016-11-12T08:14:47Z

@gyandeeps sorry for taking so long to get back. Let's hold off on this, since I'm not completely sure what the best approach is, or if this is something we should delegate to ShellJS users instead.

I'm marking this as something to look into once we get closer to v0.8.

@freitagbr feel free to chime in with your thoughts on this feature.

nfischer · 2017-02-26T08:55:48Z

After some thought, this is probably the wrong approach to solve this problem. I think it's better to leave this to users, since it can easily be solved outside of ShellJS.

nfischer requested changes Oct 20, 2016

View reviewed changes

New: nodepath option for the path

2e8ceae

nfischer self-assigned this Nov 3, 2016

nfischer requested changes Nov 3, 2016

View reviewed changes

nfischer added this to the v0.8.0 milestone Nov 12, 2016

nfischer mentioned this pull request Jan 8, 2017

refactor: create common.execPath #636

Merged

nfischer closed this Feb 26, 2017

gyandeeps deleted the issue469 branch February 26, 2017 15:28

Conversation

gyandeeps commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyandeeps Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer commented Oct 24, 2016

Uh oh!

gyandeeps commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nfischer commented Oct 25, 2016

Uh oh!

gyandeeps commented Oct 25, 2016

Uh oh!

nfischer commented Oct 25, 2016

Uh oh!

nfischer commented Nov 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyandeeps commented Nov 3, 2016

Uh oh!

nfischer commented Nov 12, 2016

Uh oh!

nfischer commented Feb 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gyandeeps commented Oct 20, 2016 •

edited

Loading

gyandeeps Oct 20, 2016 •

edited

Loading

gyandeeps commented Oct 25, 2016 •

edited

Loading