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

require does not work as described in the documentation #18695

Closed
smkoyan opened this issue Feb 10, 2018 · 7 comments
Closed

require does not work as described in the documentation #18695

smkoyan opened this issue Feb 10, 2018 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@smkoyan
Copy link

smkoyan commented Feb 10, 2018

  • v9.5.0:
  • Windows 10 Pro 64-bit:
  • modules:

I have this file structure:
image

  • app.js
    console.log( require.resolve('./hello') );

  • package.json
    { "main": "test" }

other files are empty.

run: node app.js

if rely on the documentation require should find file underlined in the picture below:
image
hello.js

but it finds this:
image
test.js

It should find file in step 3.a but it finds in step 3.b
image

@gireeshpunathil
Copy link
Member

I guess this how to interpret the module loading logic - my annotations in grey:

Input:
X=./hello
Y=/require_test

  1. If X begins with './' or '/' or '../'
    [TRUE]
    a. LOAD_AS_FILE(Y + X)
    ['/require_test' + './hello'] = ['./require_test/hello']
    b. LOAD_AS_DIRECTORY(Y + X)
  2. LOAD_NODE_MODULES(X, dirname(Y))
  3. THROW "not found"

LOAD_AS_FILE(X) [X='./require_test/hello']
[FAILS AS X is a folder now]

  1. If X is a file, load X as JavaScript text. STOP
  2. If X.js is a file, load X.js as JavaScript text. STOP
  3. If X.json is a file, parse X.json to a JavaScript Object. STOP
  4. If X.node is a file, load X.node as binary addon. STOP

LOAD_AS_DIRECTORY(X)
X=['./require_test/hello']

  1. If X/package.json is a file,
    [TRUE]
    a. Parse X/package.json, and look for "main" field.
    [TRUE], test
    b. let M = X + (json main field)
    ./require_test/hello/test
    c. LOAD_AS_FILE(M)
    DONE!

It is working as per the documentation. Closing as answered, let me know if you think otherwise.

@BridgeAR BridgeAR added the confirmed-bug Issues with confirmed bugs. label Mar 29, 2018
@richardlau
Copy link
Member

I think this is addressed by #15015?

@BridgeAR
Copy link
Member

@smkoyan this was indeed a bug and it was fixed recently. It was fixed recently by #15015.

@BridgeAR
Copy link
Member

BridgeAR commented Mar 29, 2018

@richardlau racy :D

It is handled as semver-major because it was wrong for a very long time and there is a chance that code relies on the faulty behavior. So it will only be released in v.10.0.0.

@gireeshpunathil
Copy link
Member

@BridgeAR / @richardlau - can you please clarify for my understanding? - I don't see any deviation from the doc, nor I see a doc update as part of #15015!

@richardlau
Copy link
Member

@gireeshpunathil the issue is this part of your annotations:

LOAD_AS_FILE(X) [X='./require_test/hello']
[FAILS AS X is a folder now]

This describes the incorrect behaviour, it should not fail here and X should be tried as a file. #15015 fixes the behaviour so that it matches the doc.

@gireeshpunathil
Copy link
Member

ah! ok, thanks @richardlau for the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

4 participants