Support webpack resolve.root and resolve.modulesDirectories #28

Merged
merged 1 commit into from Jan 4, 2017

Projects

None yet

2 participants

@pahen
Collaborator
pahen commented Jan 3, 2017

This will add support for resolving paths using the resolve.root option. This was needed by a project I was running madge on that was setup like this example. The default for enhanced-resolve is to resolve from node_modules so this PR will make paths in resolve.root overwriting that.

test/test.js
+ const resolved = cabinet({
+ partial: 'ast',
+ filename: `${directory}/test/ast.js`,
+ directory: `${directory}/fake/sub/dir`,
@pahen
pahen Jan 3, 2017 Collaborator

It's a little bit hard to understand this test maybe but this line will make the test fail without the patch in the resolve function.

@mrjoelkemp
mrjoelkemp Jan 3, 2017 Member

Non-blocker: I wonder if it would be better to have a separate webpack config just for this test. It would avoid any potential of breaking other tests that rely on that existing config.

@pahen
pahen Jan 3, 2017 Collaborator

Yes, sounds good. I'll fix that.

@pahen pahen requested a review from mrjoelkemp Jan 3, 2017
index.js
+ var resolveConfig = objectAssign({}, loadedConfig.resolve);
+
+ if (resolveConfig.root) {
+ resolveConfig.modules = [].concat(resolveConfig.root);
@pahen
pahen Jan 3, 2017 Collaborator

The question is if we maybe should support http://webpack.github.io/docs/configuration.html#resolve-modulesdirectories too? I suppose things could go wrong if a user has set a path in root but not node_modules. It will then fail to resolve paths in node_modules. Maybe we should always ensure node_modules is in resolveConfig.modules?

@mrjoelkemp
mrjoelkemp Jan 3, 2017 Member

I'm not sure, to be honest. We could always go the current route and add your suggestion if it becomes necessary. It's your call entirely.

@pahen
pahen Jan 3, 2017 Collaborator

I'll update to make sure node_modules is always included. Better safe than sorry :)

@mrjoelkemp
Member

Is resolve.root a proper webpack config option? (on my phone right now, so can't easily look it up)

If it is supported, then wouldn't enhanced-resolve already have this logic? We're passing the entire resolve config to that resolver.

@mrjoelkemp
Member

LGTM aside from the question above. Nice work and thanks for submitting the tested PR. <3

@pahen
Collaborator
pahen commented Jan 3, 2017

Yes, resolve.root is a valid webpack config option and my first thought was that it was supported by enhanced-resolve but it isn't. Webpack has resolve.root, resolve.modulesDirectories, and resolve.fallback but enhanced-resolve only has an option called modules.

@mrjoelkemp
Member

@pahen sounds good. I think this should be addressed upstream (mind discussing it with them), but we can definitely ship this temporary fix while we wait.

@pahen pahen changed the title from Support webpack resolve.root config option to Support webpack resolve.root and resolve.modulesDirectories Jan 4, 2017
@pahen
Collaborator
pahen commented Jan 4, 2017

@mrjoelkemp Yes, I agree it would be nice to have this fixed in enhanced-resolve. I've updated this PR now with separate webpack config, better tests, support for resolve.modulesDirectories, and a check to ensure node_modules always is in the resolve path. I think this should work pretty well :)

@pahen
Collaborator
pahen commented Jan 4, 2017

Wait with merging. I have one more upcoming fix :)

@pahen
Collaborator
pahen commented Jan 4, 2017

Strike that! I can do that in another PR.

@mrjoelkemp

LGTM

@@ -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);
@mrjoelkemp
mrjoelkemp Jan 4, 2017 Member

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.

- var resolver = webpackResolve.create.sync(loadedConfig.resolve || {});
+ var resolveConfig = objectAssign({}, loadedConfig.resolve);
+
+ resolveConfig.modules = [];
@mrjoelkemp
mrjoelkemp Jan 4, 2017 Member

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

+ });
+
+ if (!foundNodeModulesInPaths) {
+ resolveConfig.modules.push('node_modules');
@mrjoelkemp
mrjoelkemp Jan 4, 2017 Member

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.

@mrjoelkemp mrjoelkemp merged commit 48bd4b4 into dependents:master Jan 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mrjoelkemp
Member

Thanks for the updates @pahen. I'll push this up to dependency tree.

@pahen
Collaborator
pahen commented Jan 4, 2017

Sweet! I can take a look at the things you mentioned before you create a new release.

@mrjoelkemp
Member

I cut a minor release. We can always release patches to fix things up. Wanted to unblock this though.

@pahen
Collaborator
pahen commented Jan 4, 2017

Ok, fair enough :)

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