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.

Sign up for GitHub

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

Jump to bottom

doc: add note about path.basename on Windows #35065

Merged
merged 1 commit into from
Sep 10, 2020
Merged

doc: add note about path.basename on Windows #35065

merged 1 commit into from
Sep 10, 2020

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 5, 2020

As usual, Windows makes everything difficult. This adds a note to path.basename about its behavior on Windows.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. labels Sep 5, 2020
@tniessen
Copy link
Member Author

tniessen commented Sep 5, 2020

cc @nodejs/platform-windows

@addaleax
Copy link
Member

addaleax commented Sep 5, 2020

Is this behavior that we want to change? It would make sense to me to do so, given the filesystem’s behavior

@tniessen
Copy link
Member Author

tniessen commented Sep 5, 2020

@addaleax I was surprised by this behavior in Node.js, too. It turns out to be a complex problem. Technically, it's not filesystem behavior. NTFS, the standard file system on Windows, implements case-sensitive names. I never tried this, but apparently Linux code within the Windows Subsystem for Linux will be able to utilize case-sensitivity in the host system's file system. So on the filesystem level, two files with the same name, but different capitalization, can exist. However, outside of WSL, Windows cannot handle case-sensitive names, and will hide one of those files from applications. (This causes some horrific issues, e.g., microsoft/WSL#3394, microsoft/WSL#3356).

This makes it virtually impossible for Windows applications to access one of those files. I think Win32 applications would only see one of the two files, and would be unable to access the other. But if that is true, then what happens if a Win32 application moves or deletes such a file? Would the other file still exist, and now be accessible? I have no idea, it's a terrible mess.

If we change the existing API in Node.js, it would probably have to be semver-major, so landing the warning earlier might be a good idea. Another issue might be that Windows simply doesn't have an equivalent to basename, so we don't really know what the "correct" behavior would be.

@addaleax
Copy link
Member

addaleax commented Sep 5, 2020

@tniessen I’m fully agreeing :) I’m also okay with documenting this now and changing it in a semver-major update.

Trott
Trott reviewed Sep 6, 2020
View reviewed changes
doc/api/path.md Outdated Show resolved Hide resolved
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2020
@nodejs-github-bot
Copy link
Collaborator

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

Trott
Trott reviewed Sep 7, 2020
View reviewed changes
doc/api/path.md
@@ -87,6 +87,19 @@ path.basename('/foo/bar/baz/asdf/quux.html', '.html');
// Returns: 'quux'
```

Although Windows usually treats file names, including file extensions, in a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same is true on macOS. I'm not sure this is a Windows-specific thing. Do we want to change this note to be for all operating systems that treat file names as case-insensitive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, maybe it's more of an issue for Windows (since macOS users are kind of used to things being wonky in this regard, whereas Windows users may have a more consistent "paths are case-insensitive" experience)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this on macOS. Reading up on it, it seems that this is not an OS thing, but a file system thing on macOS? As in, their defautl fs is case-insensitive, but the OS also supports case-sensitive file systems? That would be a subtle difference to Windows.

We can surely adapt the note if it makes sense. I have absolutely no experience with macOS, so I'd be happy about suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine leaving the note as-is and updating can happen later if appropriate. But in case someone wants to pitch in.... @nodejs/platform-macos @nodejs/documentation

@Trott Trott force-pushed the doc-add-note-to-path-basename branch from d9983d9 to a03e15f Compare September 10, 2020 13:05
@Trott
Copy link
Member

Trott commented Sep 10, 2020

Landed in a03e15f

@tniessen @Trott
doc: add note about path.basename on Windows
a03e15f 
PR-URL: nodejs#35065
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott Trott merged commit a03e15f into nodejs:master Sep 10, 2020
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
@tniessen @ruyadorno
doc: add note about path.basename on Windows
93e4d54 
PR-URL: #35065
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@tniessen @addaleax
doc: add note about path.basename on Windows
3954b8f 
PR-URL: #35065
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@tniessen @joesepi
doc: add note about path.basename on Windows
2a30f6c 
PR-URL: nodejs#35065
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@Flarna Flarna Flarna approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tniessen @addaleax @nodejs-github-bot @Trott @lpinca @Flarna