path: assert inputs are strings by mscdex · Pull Request #5348 · nodejs/node · GitHub

This makes input checks more consistent across the path functions.

CI: https://ci.nodejs.org/job/node-test-pull-request/1712/

CI is all green except a flaky test.

LGTM

Should these changes be accompanied by a doc update?

LGTM, I suppose a doc addition stating the input type would be nice, yeah.

Documentation updated.

Maybe do it like in the buffer docs?

## path.extname(path)

* `path` {String}

description...

The docs should definitely be consistent but that can be done in a separate PR, I think. The entire path.markdown file is inconsistent with the other docs it appears.

Yeah, I realized we're pretty inconsistent with argument description all over, so disregard my comment.

This commit makes input type checking consistent across all path functions. PR-URL: nodejs#5348

One final CI: https://ci.nodejs.org/job/node-test-pull-request/1951/

This commit makes input type checking consistent across all path functions. PR-URL: #5348 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Landed in 08085c4.

This change is breaking the testing suite of both eslint + stylus

I have documented this over at eslint already: eslint/eslint#5678 (comment)
Will follow up with stylus afterwards

So it looks like the regression is some what similar to what we saw in #5244

Basically anyone who was passing null / undefined to path in the past would get an empty string back, now Node throws.

I have sent PR's to both eslint + stylus, but I can imagine that the pattern of passing undefined into a path function probably exists all over the place.

This is likely to be VERY breaking in the ecosystem (we only smoke test 48 modules, 2 of which were affected).

@nodejs/ctc assuming this is a change that we want to keep in v6, what is the best way to avoid a ton of breakages in the ecosystem?

I think the right thing to do would likely go back to the behavior of returning an empty string on null/undefined

This has come up before (#1215). We should fix for versions prior to 6, but we should use this new behavior in 6+.

so this is working as expected prior to v6. I still thinking that supplying '.' for undefined is the right thing to do. Perhaps with a deprecation message? This will be a bit out of left field for individuals moving to six with no notice, and we have no guarantee that some of the breaking modules are still maintained.

edit: I've played around with "fixing" this a little bit and I am left at a bit of an impasse. Obviously we don't want to break stuff, but being consistent on this is going to be hard. What do we do for path.join where the first argument is not a string, and the second is. In a way being strict about validating strings makes a ton of sense in this case.

Perhaps we should only be returning an empty string or '.' for functions that had one argument, but drawing that line seems pretty arbitrary.

Looking forward to hearing some more opinions

The main purpose of this PR was to create consistency. I don't have a strong feeling about which direction, but I think it should be that either all functions type check or they don't (and do implicit string conversions or whatever other solution).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

In Node.js 6 additional assertions was added in path module: nodejs/node#5348

mscdex added semver-major PRs that contain breaking changes and should be released in the next major version. path Issues and PRs related to the path subsystem. labels Feb 21, 2016

mscdex closed this Feb 21, 2016

mscdex reopened this Feb 21, 2016

mscdex force-pushed the path-assert-inputs branch from 11ffdf1 to eae3a58 Compare February 23, 2016 02:08

silverwind reviewed Feb 23, 2016
View reviewed changes

path: assert inputs are strings

779fff6

This commit makes input type checking consistent across all path functions. PR-URL: nodejs#5348

mscdex force-pushed the path-assert-inputs branch from eae3a58 to 779fff6 Compare March 18, 2016 02:42

mscdex added a commit that referenced this pull request Mar 18, 2016

path: assert inputs are strings

08085c4

This commit makes input type checking consistent across all path functions. PR-URL: #5348 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

mscdex closed this Mar 18, 2016

mscdex deleted the path-assert-inputs branch March 18, 2016 03:24

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

alberto mentioned this pull request Apr 27, 2016

Eslint raising error 'import/no-unresolved' for system modules on node v6 eslint/eslint#5981

Closed

Gobd mentioned this pull request Apr 28, 2016

Node V6 error - 'Path must be a string. Received undefined' gulp-sourcemaps/gulp-sourcemaps#192

Closed

jrmull mentioned this pull request Apr 28, 2016

--stdin causing a TypeError to be thrown with Node v6 standard/standard#504

Closed

msokk mentioned this pull request May 3, 2016

Fix Node v6 browserify/browser-resolve#80

Merged

rosszurowski mentioned this pull request May 10, 2016

Fix for Node 6.0 FWeinb/node-osascript#7

Merged

s0ph1e mentioned this pull request May 10, 2016

Fix exceptions in path.extname for node v6, close #70 website-scraper/node-website-scraper#71

Merged

anaisbetts mentioned this pull request May 13, 2016

Continue the work of Chrome 50 upgrade electron/electron#5461

Merged

10 tasks

lkuechler mentioned this pull request May 25, 2016

Node v6.2.0 patternplate/patternplate#71

Closed

DTrejo mentioned this pull request Jun 4, 2016

node-sass stops install sintaxi/harp#556

Closed

hthief mentioned this pull request Jun 17, 2016

node path api changed gruntjs/grunt-contrib-cssmin#270

Closed

michaeljota mentioned this pull request Aug 10, 2016

Error trying to use config in Node 6 sindresorhus/gulp-jasmine#81

Closed

thomasjo mentioned this pull request Aug 22, 2016

Upgrade Electron atom/atom#12300

Merged

29 tasks

ikokostya mentioned this pull request Sep 4, 2016

Fix github link generation for Node.js 6 documentationjs/documentation#524

Merged

tmcw pushed a commit to documentationjs/documentation that referenced this pull request Sep 4, 2016

Fix github link generation for Node.js 6 (#524 )

28d51fc

In Node.js 6 additional assertions was added in path module: nodejs/node#5348

oh-ren mentioned this pull request Nov 14, 2016

[NodeJS v.7] Webpack ERROR in Path must be a string. Received undefined webpack/webpack#3242

Closed

nighca mentioned this pull request Nov 21, 2016

An Webpack Error vikingmute/webpack-react-codes#4

Closed

franzliedke mentioned this pull request Feb 3, 2017

Build with Node 6 fails flarum/flarum-gulp#7

Open

ikokostya mentioned this pull request May 10, 2017

json2ts fails while read from STDIN bcherny/json-schema-to-typescript#86

Closed

path: assert inputs are strings by mscdex · Pull Request #5348 · nodejs/node · GitHub

Uh oh!

Uh oh!

path: assert inputs are strings - #5348

path: assert inputs are strings#5348mscdex wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mscdex:path-assert-inputsmscdex/io.js:path-assert-inputsCopy head branch name to clipboard

Conversation

mscdex commented Feb 21, 2016

Uh oh!

mscdex commented Feb 21, 2016

Uh oh!

mscdex commented Feb 21, 2016

Uh oh!

jasnell commented Feb 21, 2016

Uh oh!

evanlucas commented Feb 22, 2016

Uh oh!

silverwind commented Feb 22, 2016

Uh oh!

mscdex commented Feb 23, 2016

Uh oh!

silverwind Feb 23, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

mscdex commented Mar 18, 2016

Uh oh!

mscdex commented Mar 18, 2016

Uh oh!

MylesBorins commented Mar 25, 2016

Uh oh!

MylesBorins commented Mar 25, 2016

Uh oh!

jasnell commented Mar 25, 2016

Uh oh!

cjihrig commented Mar 25, 2016

Uh oh!

MylesBorins commented Mar 25, 2016

Uh oh!

mscdex commented Mar 25, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

path: assert inputs are strings#5348
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:path-assert-inputs