Update default extensions to match node

[TehShrike]

This fixes an issue where browserify/resolve's default behavior does not match node's behavior.

Per the discussion in #137, I updated the hardcoded list of extensions instead of sourcing it from some global.

Extension list sourced from https://nodejs.org/api/modules.html#modules_file_modules

Fixes #137. Closes #138. Closes #20. Closes #166.

[ljharb]

This overlaps with #20 - #20 (comment) may be relevant here.

[TehShrike]

require.extensions is used in #138 - this pull request replaces #138 per the discussion in #137, where substack and bmeck recommend against relying on the global.

[TehShrike]

Given @substack and @bmeck's comments in #137, I think this is the preferred approach over using require.extensions.

require.extensions has been deprecated since 0.10.6: https://nodejs.org/api/modules.html#modules_require_extensions

Anyone else want to chime in/review?

[ljharb]

This is not my preferred approach; either it should stay as-is, or it should dynamically look up require.extensions.

If the docs are incorrect, let’s fix them.

[TehShrike]

What's your reasoning?

The readme says "implements the node require.resolve() algorithm". Changing the readme would not change the fact that this functionality is what people expect this module to provide. The current behavior is a bug. Leaving it as-is is undesirable.

require.extensions is deprecated, and the few other people who have chimed in so far discouraged using it. I would rather we referenced require.extensions than leave this bug, but I don't see how require.extensions is the better choice over hard-coding the array to match node's documentation.

[ljharb]

The require.resolve algorithm depends on the runtime value of require.extensions; deprecated or not, it will NEVER be removed from CJS.

Hardcoding the array is a breaking change that also isn't consistent with require.resolve, so I see no value in ever doing that.

[TehShrike]

Not supporting a mutable require.extensions is not what is causing all of the issues that users are experiencing.

The issue is that the default value for this module's extensions option is different from node's default module resolution extensions.

I agree with the other commenters in #137 that inferring defaults from a global mutable is a bad idea.

The fix for the bug is to align this package's defaults with node's defaults.

[ljharb]

The fix for the bug is to make the package behave the same way node does, which means dynamically looking up require.extensions.

I'm going to continue to block any change to this package's defaults that aren't changing it to require.extensions (which I think isn't going to be worth the break, so I don't want to change that either).

[snuggs]

Not supporting a mutable require.extensions is not what is causing all of the issues that users are experiencing.

The issue is that the default value for this module's extensions option is different from node's default module resolution extensions.

Agreed. So basically this package doesn't do what it claims to do. YES? Either it uses module load strategy or it doesn't. And as @TehShrike pointed out it doesn't therefore even the name of the library is out of touch.

Not being a curmudgeon by any means. But facts don't seem to care about our opinions.

Is there another library out there I can use? It's not my fault companies decided to use a (now) deprecated feature as an early node supporter. I still have to deal with this and I'd have to drop tape due to resolve and I <3 BOTH projects. /cc @ljharb

[TehShrike]

Is there another library out there I can use?

A new module could be published, actually-node-resolve that would just export this module, but overriding the default value of extensions to ['.js', '.json', '.node'].

[snuggs]

Yes I don't think @ljharb realizes how serious this is

For people using both tools. For instance is BLOOMBERG right for using application/ecmascript in some libraries? That's debatable. As long as those mime types are in the HTML list of valid Javascript Mime types. Even though much of the world STILL uses text/javascript which is not only outdated and was labeled as OBSOLETE but is the "defacto" mime type in the HTML spec (although not even this very site we are on abides).

I feel a strawman is being thrown up telling people HOW they should write their code when the real issue after all is a project named resolve doesn't even use the Node resolve algorithm. I literally had a company state "this issue clearly shows the lib doesn't do what it says it does. It breaks our compliance and we can't use it as you suggested."

I can close whatever in respective repos as the client has moved on from resolve and tape. Unfortunate but I'd still make the changes. With people revisiting their resolution strategies (much to do with ESM / .mjs) there will be many (MANY) apps that have no issue divorcing from poor resolver strategies. It's already happening. True may be a breaking change but the reality is the ability to pass extensions was the feature/bug that's causing the problems in the first place.

On second thought actually-node-resolve isn't a bad idea @TehShrike. Would have saved two clients I have mandated by SEC from abandoning project. "What kind of 'resolve' is this because it's not node's" Became a louder cry recently than me saying "But you touched require.extensions?"

[ljharb]

The browserify issue is a different one; anyone who's using resolve directly can, and should, pass their own list of extensions - whether that's require.extensions, or ['.es'], or whatever you like. No direct consumer of resolve is blocked, in any way, from making it behave the way they like.

The issue here is deciding which of the following is preferable:

1. no change; consumers of resolve continue to provide a manual list of extensions as they like
2. change the default to a hardcoded list of the initial value of require.extensions; this is a breaking change, so some group of users will have to start providing a manual list of extensions. Anyone that mutates require.extensions (bad practice or not) will also have to provide a manual list of extensions.
3. change the default to a dynamic lookup of require.extensions. this is also a breaking change, but fewer users than in the previous option will have to provide a manual list of extensions.

My preference between 2 and 3 is decidedly "3". My overall preference is to avoid breaking changes - which cause pain for users - when they are not sufficiently motivated. I'm not yet convinced that "pedantically matching node's resolve algorithm" is actually useful enough to warrant the breaking change.

[TehShrike]

"pedantically matching node's resolve algorithm" is the whole point of this package.

[snuggs]

A breaking change to do what this library actually says the name says it does is a worthy change in my book. At least have the readme say otherwise as you suggested.

Trust me I can definitely pick up what you are laying down as well @ljharb. Like I said I no longer have a dog in the fight. People who use tape will have their hands tied behind their backs tho...if they're concerned about file extensions. Seems to be a hot topic these days. I'd prepare.

[snuggs]

P.S. To be clear I still feel @TehShrike's work SHOULD be merged in. After all we are simply devs who just want to use this library and nothing more. I thought the whole point of this project was to help people who had some janky legacy resolution coding to maintain without choice...I'm guilty as charged.

Please let me know what to do with my respective issues/PR @ljharb.

[ljharb]

@TehShrike I haven't changed my position; but for this PR to be merged, it would need tests that would fail without the change.

[snuggs]

@ljharb FWIW I know you mentioned earlier awaiting node progress. However to your point in the comment above would it be ok to do an extension lookup || array matching node docs as per your request? Would like to ensure this goes off without a hitch. Seems like no response so can either add commit to this branch or mirror another. At minimum something must be done with conflict to readme.markdown

Thanks in advance. 🙏

[TehShrike]

Added some tests. Let me know if they seem insufficient.

[ljharb]

I think it's also useful to have a test with a foo.js, foo.node, and foo.json, and specify which one gets picked with both 1) default extensions and 2) a list of two of them. I'd then want to merge that test into master, so that this PR's diff would illustrate the breaking change.

[TehShrike]

Ah yeah, asserting default extension order makes sense Josh

…

________________________________ From: Jordan Harband <notifications@github.com> Sent: Tuesday, September 11, 2018 18:16 To: browserify/resolve Cc: Josh Duff; Mention Subject: Re: [browserify/resolve] Update default extensions to match node (#145) @ljharb commented on this pull request. I think it's also useful to have a test with a foo.js, foo.node, and foo.json, and specify which one gets picked with both 1) default extensions and 2) a list of two of them. I'd then want to merge that test into master, so that this PR's diff would illustrate the breaking change. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#145 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABFsbW_xCsx_CMDZCt3LvyoHzRxlvIWfks5uaEQwgaJpZM4RTcwU>.