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

Disallow string concatenation when using __dirname and __filename (no-path-concat) #403

Closed
feross opened this Issue Feb 5, 2016 · 4 comments

Comments

2 participants
@feross
Copy link
Member

commented Feb 5, 2016

See: http://eslint.org/docs/2.0.0/rules/no-path-concat

Replace this:

var fullPath = __dirname + "/foo.js";

With this:

var fullPath = path.join(__dirname, "foo.js");

From eslint docs:

However, there are a few problems with this. First, you can't be sure what type of system the script is running on. Node.js can be run on any computer, including Windows, which uses a different path separator. It's very easy, therefore, to create an invalid path using string concatenation and assuming Unix-style separators. There's also the possibility of having double separators, or otherwise ending up with an invalid path.

In order to avoid any confusion as to how to create the correct path, Node.js provides the path module. This module uses system-specific information to always return the correct value.

@feross feross referenced this issue Feb 5, 2016

Closed

Release proposal: standard v6 #399

25 of 25 tasks complete
@feross

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2016

# tests 427
# pass  395
# fail  32

7.5% of packages either don't work on Windows, or their tests don't work on Windows.

I think we should enable this, because even though this will make people change their code, this is actually a real problem – a programmer error — in their code that should be fixed.

@feross

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2016

After fixing up all the packages that I have push access to, only 21/427 (5%) are failing. All the cases I fixed up were real cases where that code wouldn't work in a cross-platform way on Windows.

I think this is a good rule.

@jprichardson

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

ACK. I think this change makes sense.

feross added a commit to standard/standard-packages that referenced this issue Feb 6, 2016

@feross

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2016

Released in standard 6.0.0.

@feross feross closed this Feb 6, 2016

@hden hden referenced this issue Mar 17, 2016

Merged

update dependency #18

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

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