fshost / node-dir

Recursive asynchronous file and directory operations for Node.js
MIT License
221 stars 44 forks source link

Dynamic Requires in index #22

Closed osdevisnot closed 9 years ago

osdevisnot commented 9 years ago

I see index uses dynamic requires here, here and here. I could very well write same code like so:

var key,
  path = require('path'),
  dirpaths = require('./lib/paths');

for (key in dirpaths) {
  if (dirpaths.hasOwnProperty(key)) exports[key] = dirpaths[key];
}

exports.readFiles = require('./lib/readfiles');
exports.readFilesStream = require('./lib/readfilesstream');

Is there any specific reason for doing so? What purpose does it serve?

NB: Motivation for this question: trying to understand how module loaders work.

fshost commented 9 years ago

In general, ./ refers to the directory on which the node command was called. It does not refer to the directory of the file being executed, whereas dirname refers to the directory where the file being executed resides. However ./ behaves the same as dirname for require, so there is not really a reason, other than convention, why it could not be used instead. In early versions of node.js, best practice encouraged the use of path.join rather than hardcoding '/' (forward slash) in the path string to ensure compatibility with Windows, as Windows uses '\' (backslash), but the Windows version of Node.js now handles backslashes effectively, so again, it becomes just a matter of convention (unless you are passing path arguments to another Windows program.) I can't think of a reason, technically speaking, that your example code would not work just as well in the use of require. That might not be the case in other instances of working with a path however, so I tend to adhere to a convention that helps me to be mindful of that fact.

dperetti commented 9 years ago

Here is a good reason for not using those requires : they break Webpack.

WARNING in ./~/node-dir/index.js
Critical dependencies:
11:20-69 the request of a dependency is an expression
12:26-81 the request of a dependency is an expression
@ ./~/node-dir/index.js 11:20-69 12:26-81

Using the more usual requires in index.js fixes the issue :

exports.readFiles = require('./lib/readfiles');
exports.readFilesStream = require('./lib/readfilesstream');
fshost commented 9 years ago

I'd report that issue to web pack as the code is valid and should not break it.

osdevisnot commented 9 years ago

Right, I would report it to webpack. Btw, this style of require is not supported with browserify and ES6 modules.

fshost commented 9 years ago

I have simplified the require statements in index.js to use non-dynamic require statements.