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

Folders as Modules docs: Inaccuracy regarding missing main file? #22464

Closed
kfranqueiro opened this issue Aug 22, 2018 · 16 comments
Closed

Folders as Modules docs: Inaccuracy regarding missing main file? #22464

kfranqueiro opened this issue Aug 22, 2018 · 16 comments

Comments

@kfranqueiro
Copy link

  • Version: 8.11.4 and 10.9.0
  • Platform: Mac OS X
  • Subsystem: require

The Folders as Modules docs say the following, for both 8.x and 10.x:

If the file specified by the 'main' entry of package.json is missing and can not be resolved, Node.js will report the entire module as missing with the default error:

Error: Cannot find module 'some-library'

However, in practice this doesn't seem to be the whole story. It seems that it will gracefully fall back to index.js anyway if it exists.

Given the following index.js and package.json:

console.log('This is index');
{
  "name": "test",
  "main": "doesnotexist.js"
}

If you run node ., you don't get an error - you get This is index.

There is another sentence later in the docs:

If there is no package.json file present in the directory, then Node.js will attempt to load an index.js or index.node file out of that directory.

This would seem to also be applying in the case where there is a package.json, but main points to a file that doesn't exist.

Am I missing something, or do these docs need to be clarified?

PS: If you remove index.js from the above example, an error occurs but still doesn't seem to match what's in the docs - it doesn't say it can't find what's referenced by main; it simply says it cannot find the path you referenced (e.g. . rather than doesnotexist above).

TomCoded added a commit to TomCoded/node that referenced this issue Aug 23, 2018
@TomCoded
Copy link
Contributor

@kfranqueiro Does the proposed change look good?

On the PS, I am getting the "cannot find module" error specified in the docs when I remove index.js (Although the module it is attempting to find is at the working directory if you are invoking node '.' at the command line.)

@kfranqueiro
Copy link
Author

Yeah, the latest proposed change seems like it captures it. Thanks.

RE the PS, I realized just now that I was misinterpreting the docs, so you're right. Sorry for the confusion on my part there.

@ljharb
Copy link
Member

ljharb commented Aug 26, 2018

This seems like a bug - I’m reasonably sure it didn’t used to act this way. What happens in older node versions? Can we figure out when it changed?

@TomCoded
Copy link
Contributor

TomCoded commented Aug 26, 2018

Works that way for me with 1.0.0 on nvm.

If the file specified by main is missing but a default index file is present, would we prefer to throw an error?

That condition would suggest a bug in the module. Either (1) the module maintainer wants the default index file called but has the wrong main file listed in the package.json file or (2) the main file is missing from the module AND the module happens to have a file with a name matching a default index file. In case (1) the fallback would be reasonable, but in case (2) it results in undefined behavior. (Or rather, it results in accidentally-defined behavior from the POV of the module writer.)

Fixing to throw an error instead of undefined behavior would be a breaking change impacting any modules which have the bug but presently work.

@guybedford
Copy link
Contributor

Any idea from which version this started happening?

//cc @bmeck

@ljharb
Copy link
Member

ljharb commented Aug 26, 2018

I would expect that if package.json exists, and "main" is present but unresolveable, that it would throw an error. If "main" is absent or package.json is absent, then I would probably expect a default "main" to apply - ie, ./index (plus the appropriate extension resolution).

@TomCoded
Copy link
Contributor

@ljharb Yes, that sounds like a reasonable behavior.

@guybedford I'm not sure, but I tested a few versions with nvm and they all behaved this way. (The earliest I tested was 1.0.0).

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

It’d be worth it to test 0.8 and 0.10; 0.12 and 1 were when a number of things started changing.

@bmeck
Copy link
Member

bmeck commented Aug 27, 2018

To my knowledge this has always been the case. I tend to avoid "main" personally and can't remember a time where I had to have a "main". Given that all phases or searching for files catches errors and tries the next location, I'd assume this to be expected but with poor docs.

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

having an unresolvable explicit main tho doesn’t seem like it should fall back to something else.

@bmeck
Copy link
Member

bmeck commented Aug 27, 2018

@ljharb I'm neutral to this behavior, we do have precedent for EPERM failing fast though; however, this is a ENOENT error. I choose no sides on what the behavior should be. Early error seems safer though, especially given fallthrough behavior of require.

@TomCoded
Copy link
Contributor

0.12, 0.10, and 0.08 have the same behavior.

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

In that case, the behavior is consistent and we should indeed update the docs. Separately, it'd be great to update the behavior to throw when "main" is present and unresolvable.

@kfranqueiro
Copy link
Author

having an unresolvable explicit main tho doesn’t seem like it should fall back to something else.

That's what surprised me and caused me to check the docs and open this bug. IMO it would be nice to be able to detect that this is happening somehow; it's potentially more confusing for something to happen to surprisingly function but perhaps not in the intended way. But at least if we update the docs it might raise awareness of the actual behavior.

Background: I originally discovered this along with a co-worker because a repo I work on is considering updating its main to point to transpiled code under a dist folder (it currently points to index.js which is ES2015 code). Somehow our CI continued to pass as usual on a partial PR, but local tests failed. The reason why was because dist didn't exist at all when tests ran on CI, and it silently fell back to index.js and thus acted as if nothing had changed.

(I'm not trying to say my problem should be Node's problem; just giving an example of how the current silent fallback behavior can be confusing.)

@ljharb
Copy link
Member

ljharb commented Aug 27, 2018

Perhaps we could start by issuing a runtime warning when this happens.

@TomCoded
Copy link
Contributor

A run-time warning sounds like a good approach. It would let people track down the source of unexplained behavior and would warn module developers to fix their modules, but it wouldn't be a breaking change.

targos pushed a commit that referenced this issue Sep 2, 2018
PR-URL: #22494
Fixes: #22464
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: George Adams <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
PR-URL: #22494
Fixes: #22464
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: George Adams <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
PR-URL: #22494
Fixes: #22464
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: George Adams <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants