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

module: standardize strip shebang behaviour #12202

Closed

Conversation

@seppevs
Copy link
Contributor

commented Apr 4, 2017

When loading a module, Node needs to finds the end of a shebang comment by searching for a \r or \n character. This behaviour is now standardized into a dedicated internal module function

Refs: #12180

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

module

@vsemozhetbyt

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2017

@bnoordhuis
Copy link
Member

left a comment

LGTM with a style nit.

lib/internal/module.js Outdated
var contLen = content.length;
if (contLen >= 2) {
if (content.charCodeAt(0) === 35/*#*/ &&
content.charCodeAt(1) === 33/*!*/) {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 4, 2017

Member

Style nit: two more spaces of indent.

This comment has been minimized.

Copy link
@seppevs

seppevs Apr 4, 2017

Author Contributor

Fixed

module: standardize strip shebang behaviour
When loading a module, Node needs to finds the end
of a shebang comment by searching for a \r or \n character.
This behaviour is now standardized into a dedicated
internal module function

Refs: #12180

@seppevs seppevs force-pushed the seppevs:standardize_strip_shebang_behavior branch to 355449d Apr 4, 2017

if (contLen === 2) {
// Exact match
content = '';
} else {

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 4, 2017

Member

You could just drop the if (contLen === 2) case, it should almost never be hit.

@cjihrig
cjihrig approved these changes Apr 4, 2017
Copy link
Contributor

left a comment

LGTM, but I think we should add a test.

/**
* Find end of shebang line and slice it off
*/
function stripShebang(content) {

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 4, 2017

Member

stripHashBang() would be clearer

This comment has been minimized.

Copy link
@seppevs

seppevs Apr 4, 2017

Author Contributor

Shebang is more commonly used. For example, the Hashbang article on Wikipedia redirects to the Shebang article.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 4, 2017

Member

As a single data point: I've been a UNIX user for 20+ years and this is the first time I've heard someone call it a hashbang.

// Remove shebang
var contLen = content.length;
if (contLen >= 2) {
if (content.charCodeAt(0) === 35/*#*/ &&

This comment has been minimized.

Copy link
@sam-github

sam-github Apr 4, 2017

Member

does this lint? I would expect a space after 35

This comment has been minimized.

Copy link
@seppevs

seppevs Apr 4, 2017

Author Contributor

Yes it does. This code originates from lib/module.js (see master branch)

@jasnell
jasnell approved these changes Apr 4, 2017
@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

The implementation looks good to me, but I think this should have a regression test for the issue in #12180.

@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 14, 2017

Ping @seppevs – do you think you could add a test here? or, the other way around: @not-an-aardvark would you be okay with landing this and keeping #12180 open until we have a test? It would seem like an acceptable second or slightly adventurous first contribution for next week’s Code & Learn.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@not-an-aardvark would you be okay with landing this and keeping #12180 open until we have a test?

Sounds good to me.

@addaleax

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Landed in f971566

@addaleax addaleax closed this Apr 19, 2017

addaleax added a commit that referenced this pull request Apr 19, 2017
module: standardize strip shebang behaviour
When loading a module, Node needs to finds the end
of a shebang comment by searching for a \r or \n character.
This behaviour is now standardized into a dedicated
internal module function

Refs: #12180
PR-URL: #12202
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Opting to leave to bake until this has tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.