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

Support webpack resolve.root and resolve.modulesDirectories #28

Merged
merged 1 commit into from Jan 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion index.js
Expand Up @@ -12,6 +12,7 @@ var resolveDependencyPath = require('resolve-dependency-path');
var appModulePath = require('app-module-path');
var webpackResolve = require('enhanced-resolve');
var isRelative = require('is-relative-path');
var objectAssign = require('object-assign');

var defaultLookups = {
'.js': jsLookup,
Expand Down Expand Up @@ -184,7 +185,27 @@ function resolveWebpackPath(partial, filename, directory, webpackConfig) {

try {
var loadedConfig = require(webpackConfig);
var resolver = webpackResolve.create.sync(loadedConfig.resolve || {});
var resolveConfig = objectAssign({}, loadedConfig.resolve);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker: Since filing-cabinet is in the hot code path for dependency-tree, a for-in loop might be preferable. Probably negligible, but something to keep in mind for code in precinct, cabinet, and its dependencies.


resolveConfig.modules = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this blow away the existing value in resolve.modules? Shouldn't it be a default to [] if modules is empty?


if (resolveConfig.root) {
resolveConfig.modules = resolveConfig.modules.concat(resolveConfig.root);
}

if (resolveConfig.modulesDirectories) {
resolveConfig.modules = resolveConfig.modules.concat(resolveConfig.modulesDirectories);
}

var foundNodeModulesInPaths = resolveConfig.modules.some(function(dir) {
return dir.match('node_modules');
});

if (!foundNodeModulesInPaths) {
resolveConfig.modules.push('node_modules');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocker: Looking at the webpack's docs. It seems to assume that a user should know that they're responsible for supplying node_modules in modules should they choose to supply custom values.

I'd only prefer to remove this config safety net for a few reasons:

  1. https://github.com/dependents/node-filing-cabinet/pull/28/files#diff-168726dbe96b3ce427e7fedce31bb0bcR200 is in the hot code path.
  2. It would be a breaking change should we decide to remove this in the future due to perf.
  3. This also drops support for the web_modules default stated in https://webpack.github.io/docs/configuration.html#resolve-modulesdirectories.
  4. This doesn't allow a user to avoid node_modules if they wanted to.

Many of these are minor points though. I'm fine either way.

}

var resolver = webpackResolve.create.sync(resolveConfig);

// We don't care about what the loader resolves the partial to
// we only wnat the path of the resolved file
Expand Down
1 change: 1 addition & 0 deletions test/root1/mod1.js
@@ -0,0 +1 @@
module.exports = {};
1 change: 1 addition & 0 deletions test/root2/mod2.js
@@ -0,0 +1 @@
module.exports = {};
33 changes: 33 additions & 0 deletions test/test.js
Expand Up @@ -480,6 +480,39 @@ describe('filing-cabinet', function() {
assert.equal(resolved, `${directory}/node_modules/resolve/index.js`);
});

it('resolves a path using resolve.root', function() {
const resolved = cabinet({
partial: 'mod1',
filename: `${directory}/index.js`,
directory,
webpackConfig: `${directory}/webpack-root.config.js`
});

assert.equal(resolved, `${directory}/test/root1/mod1.js`);
});

it('resolves NPM module when using resolve.root', function() {
const resolved = cabinet({
partial: 'resolve',
filename: `${directory}/index.js`,
directory,
webpackConfig: `${directory}/webpack-root.config.js`
});

assert.equal(resolved, `${directory}/node_modules/resolve/index.js`);
});

it('resolves a path using resolve.modulesDirectories', function() {
const resolved = cabinet({
partial: 'mod2',
filename: `${directory}/index.js`,
directory,
webpackConfig: `${directory}/webpack-root.config.js`
});

assert.equal(resolved, `${directory}/test/root2/mod2.js`);
});

it('resolves files with a .jsx extension', function() {
testResolution('./test/foo.jsx', `${directory}/test/foo.jsx`);
});
Expand Down
11 changes: 11 additions & 0 deletions webpack-root.config.js
@@ -0,0 +1,11 @@
var path = require('path');

module.exports = {
entry: "./index.js",
resolve: {
modulesDirectories: ['test/root1'],
root: [
path.resolve(__dirname, './test/root2')
]
}
};