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

@feross
Copy link
Member

feross 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
Copy link
Member Author

feross 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
Copy link
Member Author

feross 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
Copy link
Member

ACK. I think this change makes sense.

feross added a commit to standard/standard-packages that referenced this issue Feb 6, 2016
@feross
Copy link
Member Author

feross commented Feb 6, 2016

Released in standard 6.0.0.

@feross feross closed this as completed Feb 6, 2016
@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.
Projects
Archived in project
Development

No branches or pull requests

2 participants