Shebang parsing is slightly different when using the --check flag

[not-an-aardvark]

• Version: 7.8.0 (although this has existed since the --check flag was introduced in 5.0.0)
• Platform: macOS
• Subsystem: cli

When loading a module, Node finds the end of a shebang comment by searching for a \r or \n character.

When using the --check flag, Node finds the end of a shebang comment by searching for a character that is not matched by . in a regex.

The characters \u2028 and \u2029 are considered linebreaks in JS, so they aren't matched by . in a regex. As a result, a file with \u2028 or \u2029 in the shebang will load successfully as a module, but will cause an error when parsed with the --check flag.

Example file (contains \u2028 after bin):

#!/usr/bin
/env node
console.log("foo")

The same thing applies when piping code from stdin.

[TimothyGu]

We should probably have a require('internal/module').stripShebang to standardize the behavior.

[gibfahn]

Adding good first contribution, although it's more of a second or third contribution. The change landed in #12202, what's needed here is a test or tests for the behaviour.

[guylil]

Hi,

I am pretty new to contributing here,
Where should I put the test?
Thanks!
@gibfahn

[TimothyGu]

@guylil Actually sorry, this issue was already fixed in #12202 but we forgot to close it. Doing that now.

[not-an-aardvark]

We decided to leave it open because the fix didn't have any tests. #12202 (comment)

[TimothyGu]

@not-an-aardvark Ah oops.

[guylil]

Hi, Thanks for the comments, I am a member of a meetup group called #goodneesSquad that meets and contributes to open source projects I am new to node, we did understand the issue but we couldn't find out where to put the test... So, just a path for the js file will be great, Many thanks! On Jul 20, 2017 4:57 AM, "Timothy Gu" <[email protected]> wrote: @not-an-aardvark <https://github.com/not-an-aardvark> Ah oops. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXQT5ns_X7LcFz9jwOBKvhGm-zbDvZTHks5sPrPzgaJpZM4MxOg3> .

[not-an-aardvark]

@guylil Thanks for contributing! I think there are a few options for where to put the test: You could either create a new file in test/parallel/, or you could add the test to an existing file, such as test/parallel/test-cli-eval.js.

There is also a guide on writing tests, which can be found here.

[gibfahn]

I am pretty new to contributing here,
Where should I put the test?
Thanks!

So if the test can be run in parallel (if you're not sure, it probably can) then it should go into test/parallel/.

First thing to read/follow is CONTRIBUTING.md, there's also a neat guide to writing tests. Looking at other tests in that directory may help too.

The code that needs tests was added in #12202 (which landed in f971566), so looking at that should be helpful.

Once you have something feel free to raise a PR (You can start the title with WIP - if it's still a Work in Progress). Once you've opened a PR people can help out by suggesting changes.

If you have any more questions feel free to ask them here.

[guylil]

Thank you very much!

…

On Jul 20, 2017 11:17 AM, "Gibson Fahnestock" ***@***.***> wrote: I am pretty new to contributing here, Where should I put the test? Thanks! So if the test can be run in parallel (if you're not sure, it probably can) then it should go into test/parallel/ <https://github.com/nodejs/node/tree/master/test/parallel>. First thing to read/follow is CONTRIBUTING.md <https://github.com/nodejs/node/blob/master/CONTRIBUTING.md>, there's also a neat guide to writing tests <https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md>. Looking at other tests in that directory may help too. The code that needs tests was added in #12202 <#12202> (which landed in f971566 <f971566>), so looking at that should be helpful. Once you have something feel free to raise a PR (You can start the title with WIP - if it's still a Work in Progress). Once you've opened a PR people can help out by suggesting changes. If you have any more questions feel free to ask them here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXQT5svKISCxn6Pm2nhs3JnOPOoelnOIks5sPwz8gaJpZM4MxOg3> .

[ayazhafiz]

@guylil are you still on this? Otherwise, I'd like to work on this.

[not-an-aardvark]

Looking at this again, I think it might have already been fixed by 83ebb6d.