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

doc: require behavior on case-insensitive systems #5331

Closed
wants to merge 1 commit into from

Conversation

hgwood
Copy link
Contributor

@hgwood hgwood commented Feb 19, 2016

See #5143 and nodejs/node-v0.x-archive#6829.

This adds a paragraph in the Module Caching Caveats section about the behavior of require when Node is running on top of a file system (e.g. HFS) or operating system (e.g. Windows) that will not consider the case of file paths to find files.

The goal is to make the behavior unambiguously clear, even though the previous paragraph might hint to the same caveat (to be honest, I'm not sure I understand that previous paragraph).

What do you think?

And thanks for making Node!

@ChALkeR ChALkeR added doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem. labels Feb 19, 2016
@jasnell
Copy link
Member

jasnell commented Feb 19, 2016

/cc @nodejs/ctc

@evanlucas
Copy link
Contributor

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Feb 21, 2016

LGTM

@orangemocha
Copy link
Contributor

LGTM, though I think we should make the cache case insensitive as well. In any case, the documentation change is good given the current behavior.

resolved filenames can point to the same file, but the cache will still treat
them as different modules and will reload the file multiple times. For example,
`require('./foo')` and `require('./FOO')` return two different objects,
unrespective of whether or not `./foo` and `./FOO` are the same file.
Copy link
Member

Choose a reason for hiding this comment

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

s/unrespective/irrespective

@rvagg
Copy link
Member

rvagg commented Feb 23, 2016

I would be inclined to just say "file systems" and leave out "operating systems" but does Windows break this rule given that NTFS is technically not case-insensitive (is it?)?

I believe the previous paragraph is limiting its comment entirely to where a module with a given name is located in the node_modules hierarchy given that it's possible to have an identical package located at multiple places, being resolved differently from other packages depending on where they are situated in relation to those packages. Less of a problem now with npm@3 given a more aggressive approach to hierarchy flattening but it's still perfectly reasonable to find two packages of different versions being resolved differently by dependents even though they use the same require('foo') in the same application.

lgtm sans spelling nit and resolution of my question above re "operating systems"

@hgwood
Copy link
Contributor Author

hgwood commented Feb 23, 2016

@rvagg

  • I think you are correct about the previous paragraph. Thanks to @bnoordhuis I understand it better now.
  • You are also right about the spelling. I will fix it.
  • On the FS/OS matter, Windows indeed provides a case-insensitive API over any file system, including NTFS, which is POSIX-compliant and case-sensitive (source).

@orangemocha
Copy link
Contributor

I believe you are correct about Windows. NTFS can be made case sensitive but Win32 is case-insensitive. So I think mentioning "operating systems" makes sense.

@bnoordhuis
Copy link
Member

As another data point: the ntfs-3g driver for linux is case-sensitive by default.

This adds a paragraph in the Module Caching Caveats section about the
behavior of require when Node is running on top of a file system (e.g.
HFS) or operating system (e.g. Windows) that will not consider the case
of file paths to find files.
@hgwood
Copy link
Contributor Author

hgwood commented Feb 23, 2016

@rvagg Spelling fixed.

@silverwind
Copy link
Contributor

LGTM, landed in 5298c81.

@silverwind silverwind closed this Feb 27, 2016
@hgwood hgwood deleted the fix/issue_5143 branch February 27, 2016 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants