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

resolve does not preserve require.extensions list and assumes only ['.js'] #137

Closed
snuggs opened this issue Sep 22, 2017 · 8 comments · Fixed by tape-testing/tape#396 · May be fixed by #145
Closed

resolve does not preserve require.extensions list and assumes only ['.js'] #137

snuggs opened this issue Sep 22, 2017 · 8 comments · Fixed by tape-testing/tape#396 · May be fixed by #145

Comments

@snuggs
Copy link

snuggs commented Sep 22, 2017

Greetings!

Having a situation where we are using tape which does not pass extensions to resolve. Have opened an issue here tape-testing/tape#395

That being said our situation is we use the .es ECMAScript extension with the following:

require.extensions ['.es'] = require.extensions ['.js']

Although we can achieve our needs with the previous tape issue/pr I feel it behoves resolve to use the default require.extensions list provided by node instead of only assuming ['.js'] here

var extensions = opts.extensions || ['.js'];
.

An easy enhancement would be to convert:

var extensions = opts.extensions || ['.js'];

to

var extensions = opts.extensions
  || Object.keys (require.extensions)

Please advise and will have a PR & tests up ASAP as we need this or have to call off using tape and resolve. Can also enhance documentation which should close #134 which @ljharb, and @call-a3 are working on as well.

Thanks in advance! /cc @brandondees

@ljharb
Copy link
Member

ljharb commented Sep 22, 2017

This would be a breaking change; require.extensions starts with .js, .json, and .node.

You can certainly use resolve with require.extensions via the extensions option, however.

@TehShrike
Copy link

If this package's goal is to implement the node.js require.resolve() algorithm, isn't this something that should be fixed?

Using a truncated default extensions list makes this library default to non-node-like behavior.

Packages like got that require('./package') will break when bundled by bundlers that use node-resolve (which is all bundlers that I'm aware of).

@bmeck
Copy link

bmeck commented Sep 28, 2017

It should be noted that both NODE_PATH and require.extensions have been deprecated for years. They also do not affect the ESM resolution algorithm. I would recommend moving off of relying on their usage.

@ghost
Copy link

ghost commented Sep 28, 2017

I think it's good to not rely too heavily on global runtime vars to configure default values. The whole point of this package instead of using require.resolve() is that you can configure its behavior without mutating the global runtime environment variables.

@TehShrike
Copy link

That makes sense to me. Changing the hardcoded default to match the extensions given in the docs seems preferable.

@snuggs
Copy link
Author

snuggs commented Sep 29, 2017

@TehShrike hence https://github.com/browserify/resolve/pull/138/files

This way even if someone is on some ancient deprecated api that's technically superfluous to the intent of what the library is assumed to accomplish. Feels quite awkward one would have to pass in the standard require.extensions to override the default require resolve "extensions". P.O.L.A. is real.

@TehShrike
Copy link

Interested parties should go review #145.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2018

Closing this in favor of #166, to centralize further discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants