This repository has been archived by the owner. It is now read-only.

module name resolution order insufficiently specified #5430

Closed
loveencounterflow opened this Issue May 8, 2013 · 8 comments

Comments

Projects
None yet
4 participants
@loveencounterflow

i've always found one weak spot in the node docs was how node deals with a situation where there are multiple candidate files by the same name, but different extensions.

the docs do state that "If the exact filename is not found, then node will attempt to load the required filename with the added extension of .js, .json, and then .node", but they do not explain what happens after more entries are present in require.extensions.

furthermore, the pseudocode clarification leaves out the .json case, and does also not state what happens with the other extensions.

i have a question about this on stackoverflow (see http://stackoverflow.com/questions/16427654/order-of-resolution-with-nodejs-require-module), and it would appear that, in the absence of any list of extensions to specify ordering, nodejs would probably rely on attribute order as results from a for ( var name in property.extensions ) loop or similar device; now it so turns out (per http://javascriptweblog.wordpress.com/2011/01/04/exploring-javascript-for-in-loops/) that that ordering is (1) un(der)specified in JS, being conventional rather than prescriptive; (2) v8 treats edge cases (numerical and numerical-literal object keys) differently than most engines (see
http://code.google.com/p/v8/issues/detail?id=164#c17 and
http://code.google.com/p/chromium/issues/detail?id=883#c12).

ok so i did this test:

  • since js engines commonly iterate over object keys in order of their assignment, adding a new extension should make that new one come late in the chain, so .js being an early bird, latecomers cannot replace it in the food chain. that would apply to re-assignments as well, so no chance to break into the chain by require.extensions[ '.js' ].
  • BUT extensions have their dot explicitly listed in require.extensions, so you can assign a handler to the key, say xxx (making it possible for require 'foo' to resolve to require fooxxx, a perhaps undesirable thing to ever do, but who knows).
  • and since the above holds, and v8 treats keys consisting of ascii digits ^[0-9]+$ different from other keys, suddenly you can define require.extensions[ '123' ] and make require 'foo' resolve to foo123 in preference over foo.js.

this is, granted, a weird / undesirable / edge-casey thing, but given that name resolution order is important to maintain predictability, it is worth consideration imho.

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 8, 2013

I would be ok with a doc patch explaining that the order of checking is based on the enumeration order of object keys, and thus defined by V8 and not by Node.

People should not be using require.extensions. It's officially deprecated, and the docs for the next v0.10 release will state this. I'd actually suggest that in master, we wrap it in a warning. It was a mistake, and is a constant source of bugs.

isaacs commented May 8, 2013

I would be ok with a doc patch explaining that the order of checking is based on the enumeration order of object keys, and thus defined by V8 and not by Node.

People should not be using require.extensions. It's officially deprecated, and the docs for the next v0.10 release will state this. I'd actually suggest that in master, we wrap it in a warning. It was a mistake, and is a constant source of bugs.

@loveencounterflow

This comment has been minimized.

Show comment
Hide comment
@loveencounterflow

loveencounterflow May 8, 2013

ok i am doing this so i can introduce new filetypes like coffeescript did. what is the accepted future way in adding custom extensions then?

@1 i'm totally ok with just a few words of clarification in the docs

ok i am doing this so i can introduce new filetypes like coffeescript did. what is the accepted future way in adding custom extensions then?

@1 i'm totally ok with just a few words of clarification in the docs

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 8, 2013

Compile your code to JavaScript prior to running it. Use a prepublish script to do this in your packages before publishing them to npm.

There is never any need for additional filetype extensions. Node runs JavaScript. Ultimately, you have to compile it to JS before it runs anyway, so you may as well do that up front, rather than adding a global hook for it.

isaacs commented May 8, 2013

Compile your code to JavaScript prior to running it. Use a prepublish script to do this in your packages before publishing them to npm.

There is never any need for additional filetype extensions. Node runs JavaScript. Ultimately, you have to compile it to JS before it runs anyway, so you may as well do that up front, rather than adding a global hook for it.

@loveencounterflow

This comment has been minimized.

Show comment
Hide comment
@loveencounterflow

loveencounterflow May 8, 2013

yeah, i'm afraid i think this is one great thing that you can require 'any' language in nodejs. are you sure you want to kill all the wonderful functionality / flexible cross-language interoperability that comes with it? i can't believe this! what are the bugs you speak about?

yeah, i'm afraid i think this is one great thing that you can require 'any' language in nodejs. are you sure you want to kill all the wonderful functionality / flexible cross-language interoperability that comes with it? i can't believe this! what are the bugs you speak about?

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo May 9, 2013

@loveencounterflow allowing direct execution of non JS programs in Node.js will attract disruption, which will do just harm.

NPM should be a center point (a package manager for JavaScript) where we use one language, and we're not bothered with tens of other dialects that require compilation and are written in code that's not readable for JavaScript programmer.

I'd say it's very wise decision on part of @isaacs that he doesn't lead Node.js/NPM in direction you asking for.

medikoo commented May 9, 2013

@loveencounterflow allowing direct execution of non JS programs in Node.js will attract disruption, which will do just harm.

NPM should be a center point (a package manager for JavaScript) where we use one language, and we're not bothered with tens of other dialects that require compilation and are written in code that's not readable for JavaScript programmer.

I'd say it's very wise decision on part of @isaacs that he doesn't lead Node.js/NPM in direction you asking for.

@loveencounterflow

This comment has been minimized.

Show comment
Hide comment
@loveencounterflow

loveencounterflow May 9, 2013

if it is all that bad, why is the feature in nodejs at all? and if you guys are about to rip it out, where is the anouncement? and what will the coffeescript people do?

my guess is they will build a replacement for require, and it will work as it used to, minus the fine conventionalized standardization that we used to have.

if it is all that bad, why is the feature in nodejs at all? and if you guys are about to rip it out, where is the anouncement? and what will the coffeescript people do?

my guess is they will build a replacement for require, and it will work as it used to, minus the fine conventionalized standardization that we used to have.

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 9, 2013

if it is all that bad, why is the feature in nodejs at all?

Because we are human, and make mistakes.

and if you guys are about to rip it out, where is the anouncement?

We aren't going to rip it out until we know we can do so with minimal disruption. The plan is to search all packages in npm, and warn authors directly, and also place warnings on npmjs.org pages for packages that rely on this, indicating that they will eventually cease to function. It's a multiple-stage process, and there are tools that I'm waiting to finish being built and deployed so that I can use to do this in a distributed fashion.

and what will the coffeescript people do?

Hopefully stop relying on this horrible feature. I sent a pull request, but I can't seem to find it. Maybe @jashkenas knows?

my guess is they will build a replacement for require

Or they'll just compile their CoffeeScript files to JavaScript, like they already can do now quite easily.

No one seems to be stepping up to add the doc patches, I'm closing this issue, because there's really nothing new to discuss, and it's not a priority.

isaacs commented May 9, 2013

if it is all that bad, why is the feature in nodejs at all?

Because we are human, and make mistakes.

and if you guys are about to rip it out, where is the anouncement?

We aren't going to rip it out until we know we can do so with minimal disruption. The plan is to search all packages in npm, and warn authors directly, and also place warnings on npmjs.org pages for packages that rely on this, indicating that they will eventually cease to function. It's a multiple-stage process, and there are tools that I'm waiting to finish being built and deployed so that I can use to do this in a distributed fashion.

and what will the coffeescript people do?

Hopefully stop relying on this horrible feature. I sent a pull request, but I can't seem to find it. Maybe @jashkenas knows?

my guess is they will build a replacement for require

Or they'll just compile their CoffeeScript files to JavaScript, like they already can do now quite easily.

No one seems to be stepping up to add the doc patches, I'm closing this issue, because there's really nothing new to discuss, and it's not a priority.

@isaacs isaacs closed this May 9, 2013

@TooTallNate TooTallNate referenced this issue in Automattic/browserbuild May 10, 2013

Closed

CoffeeScript support #31

@rlidwka

This comment has been minimized.

Show comment
Hide comment
@rlidwka

rlidwka May 11, 2013

NPM should be a center point (a package manager for JavaScript) where we use one language, and we're not bothered with tens of other dialects that require compilation and are written in code that's not readable for JavaScript programmer.

Did you even see compiled coffeescript files? Despite all best efforts they are not readable for anyone but JS parser. Things like var I, _i; for (I = _i = 0; _i <= 2; I = ++_i) {} are just a start.

The is only one reason why coffeescript source files have no place in npm registry. Performance.

But it's not about npm registry, require.extensions are primarily useful in development, and removing require.extensions will hurt very much here unless a suitable replacement will be introduced.

rlidwka commented May 11, 2013

NPM should be a center point (a package manager for JavaScript) where we use one language, and we're not bothered with tens of other dialects that require compilation and are written in code that's not readable for JavaScript programmer.

Did you even see compiled coffeescript files? Despite all best efforts they are not readable for anyone but JS parser. Things like var I, _i; for (I = _i = 0; _i <= 2; I = ++_i) {} are just a start.

The is only one reason why coffeescript source files have no place in npm registry. Performance.

But it's not about npm registry, require.extensions are primarily useful in development, and removing require.extensions will hurt very much here unless a suitable replacement will be introduced.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.