Fix npm issue #3380.

[basbossink]

As discussed I created a fix for the above mentioned issue. I would very much appreciate any feedback, on issues of style and/or naming conventions since this is my first delve into node. I hope you see some value in these changes and accept this request.

Fix npm issue #3380 by changing the following:

• Adapt the regular expression used to match the first line.
• Create a simple module that can convert shell style variable declarations: NODE_PATH=./lib:$NODE_PATH to the equivalent batch syntax: @SET=NODE_PATH=./lib:%NODE_PATH%. Furthermore the structure of the generated shim now looks like this:

    @SETLOCAL
    <variable declarations>
    <original content>
    @ENDLOCAL

• Note that the new segments are only added to the file if there were any variable declarations.
• The generated shell script carries over the captured variable declaration as is.
• Add some extra tests to validate the behavior.
• Remove some unnecessary white-space in the generated shim.

[ForbesLindesay]

Cool. This repo is ; free wherever possible, so it would be good if you could remove all the un-necessary semicolons. They're pretty much only actually needed in for loops.

I'd like ./toBatchSyntax.js to be in ./lib/to-batch-syntax.js, firstly because I like to avoid cluttering up the root folder, and secondly because different OSes treat case sensitivity of file names differently so it's easier to just keep everything in lower case.

I'll also need someone else to help review this and make sure I haven't missed anything. I'm not too hot on Unix shell scripts :s. Hopefully @isaacs or @domenic can take a look :smile:

[isaacs]

So, I see what you're doing here. Preserving environment variables from the shebang line into the program seems reasonable, and that's what this approach seems to do. From that point of view, lgtm.

However, the use case is pretty wacky. Adding ./lib to the NODE_PATH probably doesn't do what you think it does, and it will have wide-reaching global affects on any modules you load from that script!

The reason for this is that the NODE_PATH environ is not rooted on the cwd. Also, if NODE_PATH was not already set, you're not just setting it to ./lib, you're setting it to ./lib:, or in other words, ['./lib', '']. That empty string will cause it to auto-load modules from the module's directory. If you load a dependency, and it does an unqualified require() to try to load a dependency, and this also matches a module in its local folder, then strange things will happen.

The NODE_PATH environment variable should only ever be set to a full path. If your goal is to remove ./lib from your require() calls, then consider the costs of what you're doing here. You are creating subtle bugs for yourself in the future.

[basbossink]

I agree with @isaacs that the use case is pretty wacky. That the specific example is using the feature of being able to define environment variables in the wrong way is true, I do think however that the feature could be used in a meaningful way.