Implements `useProcessResolution` #170

arcanis · 2018-10-19T13:22:43Z

This PR implements the useProcessResolution option we discussed with @ljharb. I've split the feature in three commits:

The first one simply extends the options to allow 1/ the node resolution algorithm to be optionally disabled, and 2/ to compute extraneous paths based on the starting directory (rather than only a fixed list).
The second one slightly changes the internals so that the nodeModulesPaths function has access to the request being processed. This makes it possible to compute extraneous paths based on the request itself (not only on the starting directory).
The third one wraps the feature by implementing a useProcessResolution option that tweaks the option set to match what should be used by the current process (in this case, I've added builtin support for Plug'n'Play).

As you can see, the feature is quite self-contained and generic enough to be used in other cases than simply Plug'n'Play. It should be compatible with both pathFilter and packageFilter, since it happens earlier in the resolution process.

arcanis · 2018-10-19T16:13:11Z

I checked the Travis jobs, the failure don't seem related to this PR (hard to say since there isn't the log is messed in both cases, but the first one seem to be a compilation error and the second one a setup timeout)?

arcanis · 2018-10-25T20:55:37Z

Ping @ljharb? 🙂

ljharb · 2018-10-26T00:16:54Z

Sorry for the delay :-) indeed, the travis failure is just a fluke; I've re-ran it. The linter is failing, however.

ljharb

It seems like this is adding three new options:

useNodeModules - a boolean to control whether the standard node resolution is used
"paths" as a function - instead of an array of paths, this is a function that produces an array of paths?
"useProcessResolution" - a boolean that sets up all these options to a second set of defaults.

the latter part seems problematic, because this seems like pnp-specific code living in resolve :-/

ljharb · 2018-10-26T00:17:37Z

lib/node-modules-paths.js

@@ -2,7 +2,7 @@ var path = require('path');
 var fs = require('fs');
 var parse = path.parse || require('path-parse');

-module.exports = function nodeModulesPaths(start, opts) {
+module.exports = function nodeModulesPaths(x, start, opts) {


changing the signature in this way makes it a breaking change; also, what is "x"?

can we make this be the last argument instead?

I wasn't sure if it was a publicly exposed function, since it wasn't listed in the doc.

x is the request being made (took the variable name from other parts of the code). It can easily be moved to the end of the argument list, for sure

Everything reachable is part of the public API; docs are irrelevant - so any break in anything that's exposed would be semver-major.

ljharb · 2018-10-26T00:18:37Z

lib/node-modules-paths.js

-    }
+    var dirs = [];
+
+    if (!opts || typeof opts.useNodeModules === 'undefined' || opts.useNodeModules) {


boolean options should default to false, so that absence and false are the same - let's invert the naming of this option.

it'd also be nice to minimize the git diffs if possible.

Renamed the option - minimizing the diff would involve doing unnecessary operations at runtime (or breaking the indentation), I'd like to avoid that :/

it seems like we could maybe use early returns here, or extract pieces into helper functions?

I'm not sure it would drastically improve the code, but if you feel strongly I can do it. What I like with the current formatting is that the order in which the transformations are applied is quite clear - a random user can easily follow it from beginning to end. Early return could make this slightly harder, what do you think?

readme.markdown

ljharb · 2018-10-26T00:23:15Z

test/node-modules-paths.js

@@ -7,6 +7,11 @@ var nodeModulesPaths = require('../lib/node-modules-paths');

 var verifyDirs = function verifyDirs(t, start, dirs, moduleDirectories, paths) {
    var moduleDirs = [].concat(moduleDirectories || 'node_modules');
+    if (paths) {
+        for (var k = 0; k < paths.length; ++k) {
+            moduleDirs.push(path.basename(paths[k]));


this seems like a behavior change - the test is now using path.basename on all the paths instead of passing them in directly.

Only one test was using the path parameter previously and it was providing straight folder names (['a', 'b']), so it shouldn't change what's being tested

Fair, but why is the basename change needed? Was it incorrect for the test to omit it previously (and there weren't enough test cases to demonstrate that)?

Oops, missed the comment, my bad! The basename change was needed because the previous testcases were only using paths to contain single-component paths. Now that there's a test that adds an absolute path, we must be careful not to add the full absolute path into the moduleDirs variable.

arcanis · 2018-10-26T11:20:08Z

Applied the requested changes!

the latter part seems problematic, because this seems like pnp-specific code living in resolve :-/

Yeah, I understand the concern, it can potentially add to the maintenance burden. The way I see it:

The alternative would be to add such a code as a special case to resolve when loaded through the PnP API (through a wrapper). It could work, but I'm not too fond of wrappers ... they are much more invasive and hard to get right (what if the users use a different resolve than yours, for example an unrelated git package?), and they ever so slightly increase the .pnp.js size for all users, including those that don't use resolve.
- Note that it would actually be easier for me if it was a wrapper 😃 since I wouldn't have to go outside and talk with the people that use resolve to teach them to use useProcessResolution if it matches the behavior they expect. I just don't think it's the best way on the long term.
The code is very self-contained, as you can see. It doesn't bleed anywhere outside of the process.versions.pnp guard, so only PnP users will ever be affected at runtime by it (users for which resolve would simply not work otherwise, so it's a strict improvement in this regard).
I wouldn't oppose removing this code later on if you think PnP shouldn't be supported anymore (for lack of adoption or otherwise). In the meantime, I'll be there to maintain it if there's any issue with it. This isn't a "drop a turd and leave" situation 🙂

Your call on this part. I can easily remove it from the diff and just leave the useProcessResolution option (I think I would need it anyway to detect which resolve calls should be wrapped).

arcanis · 2018-11-02T16:26:20Z

Ping @ljharb? 😊

arcanis · 2018-11-14T11:55:18Z

Closing in favor of the sub-PRs #172, #173, #174

arcanis added 4 commits October 19, 2018 13:10

Implements paths=function and useNodeModules=false

e011edc

Makes nodeModulesPaths accept the request

5f11711

Implements useProcessResolution

109f275

Adds documentation

5797755

Merge branch 'master' into pnp-compat

5655d0a

ljharb reviewed Oct 26, 2018

View reviewed changes

Maël Nison added 2 commits October 26, 2018 12:00

Makes the request an option rather than a parameter

c9faab9

Renames useNodeModules into skipNodeModules

9daf6b4

rwjblue mentioned this pull request Oct 31, 2018

Yarn v2 / PnP Support ember-cli/ember-cli#8164

Closed

6 tasks

arcanis mentioned this pull request Nov 8, 2018

Yarn Plug'n'Play support for babel-node babel/babel#8967

Closed

rwjblue mentioned this pull request Nov 11, 2018

Yarn PnP Support quaertym/ember-cli-dependency-checker#92

Closed

This was referenced Nov 14, 2018

Adds support for "paths" being a function #172

Merged

Implements skipNodeModules #173

Merged

Implements a "normalize-options" pseudo-hook #174

Merged

arcanis closed this Nov 14, 2018

This was referenced Mar 10, 2019

Yarn PnP compatibility jsx-eslint/eslint-plugin-react#2192

Closed

Yarn pnp compatibility facebook/create-react-app#6605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements `useProcessResolution` #170

Implements `useProcessResolution` #170

arcanis commented Oct 19, 2018 •

edited

Loading

arcanis commented Oct 19, 2018

arcanis commented Oct 25, 2018

ljharb commented Oct 26, 2018

ljharb left a comment

ljharb Oct 26, 2018

arcanis Oct 26, 2018

ljharb Oct 29, 2018

ljharb Oct 26, 2018

arcanis Oct 26, 2018

ljharb Oct 29, 2018

arcanis Nov 2, 2018

ljharb Oct 26, 2018

arcanis Oct 26, 2018

ljharb Oct 29, 2018

arcanis Nov 2, 2018

arcanis commented Oct 26, 2018

arcanis commented Nov 2, 2018

arcanis commented Nov 14, 2018

Implements useProcessResolution #170

Implements useProcessResolution #170

Conversation

arcanis commented Oct 19, 2018 • edited Loading

arcanis commented Oct 19, 2018

arcanis commented Oct 25, 2018

ljharb commented Oct 26, 2018

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Oct 26, 2018

arcanis commented Nov 2, 2018

arcanis commented Nov 14, 2018

Implements `useProcessResolution` #170

Implements `useProcessResolution` #170

arcanis commented Oct 19, 2018 •

edited

Loading