Make it so relative proxyquired paths are relative to the module being required. #190

jwalton · 2018-03-01T04:32:19Z

Fixes #189.

I added a test case that fails without this fix, and passes with it.

This should definitely be a major version rev. It's possible someone is currently doing something like:

// src/a.js
require('../b.js');

// b.js
require('./c.js');

// test.js
proxyquire('./src/a.js', {'./c.js': ...});

Which would no longer work with this change. I can do a writeup for the README.md that explains this breaking change.

jwalton · 2018-03-01T04:35:03Z

lib/proxyquire.js

+// be resolved.
+Proxyquire.prototype._resolveRelativePath = function(path, moduleFilename) {
+  try {
+    if(path[0] === '.') {


Perhaps I'm overthinking this, and we should just resolve everything, instead of only the relative paths. Then we wouldn't have to resolve them all a second time in the cleanup part of _withoutCache()... Yeah, I think I'm going to make that change.

Except, when I try this it breaks a bunch of unit tests. Maybe I'll leave it alone. :)

bendrucker

Until there is only a single place for resolving paths, I don't think I'm ready to merge this.

There's also no need to prepend resolved to stubs. It may seem clear but it just removes clarity from the functions that have both stubs and resolvedStubs in scope.

bendrucker · 2018-03-01T16:26:55Z

package.json

@@ -4,7 +4,8 @@
  "description": "Proxies nodejs require in order to allow overriding dependencies during testing.",
  "main": "index.js",
  "scripts": {
-    "test": "mocha"
+    "test": "mocha",
+    "lint": "jshint index.js lib"


Hey please don't do this, contributing a linter in an unrelated PR is quite unwelcome

Oh, I noticed there was a .jshintrc in the root folder. :P I'll pull it out.

Yeah yeah my fault I didn't even realize there was jshintrc stuff in there. Since it hasn't been in CI for a while I'm just going to replace it with something a bit more ambitious/opinionated that's actually catching bugs.

bendrucker · 2018-03-01T16:28:48Z

lib/proxyquire.js

+// be resolved.
+Proxyquire.prototype._resolveRelativePath = function(path, moduleFilename) {
+  try {
+    if(path[0] === '.') {


bendrucker · 2018-03-01T16:31:38Z

lib/proxyquire.js

+  var resolvedPath = Module._resolveFilename(path, module);
+  var self = this;
+
+  var resolvedStubs = {};


Try not to assign into objects as a side effect like this. stubs = Object.keys(stubs).reduce(reducer, {}).

bendrucker · 2018-03-01T16:35:47Z

lib/proxyquire.js

 Proxyquire.prototype._withoutCache = function(module, stubs, path, func) {
  // Temporarily disable the cache - either per-module or globally if we have global stubs
  var restoreCache = this._disableCache(module, path);
+  var resolvedPath = Module._resolveFilename(path, module);
+  var self = this;


Don't use self = this except when necessary. [].reduce and its derivatives (e.g. map) take a thisArg as their last argument.

Really? The MDN page doesn't mention this. In ES6 I would just use an arrow function. I suppose I could just use 'bind'.

Hmm I guess it's only reduce that's missing it. It has an extra arg (the initial value) but I always consider it the "base" list function since it's the only one you can use to make all the others without loops or closures. Feel free to use self = this for [].reduce or bind. No perf concern here so either is ok.

I wouldn't mind adopting some light ES6 syntax (node 4 compatible) sometime in the future but let's not change that now.

bendrucker · 2018-03-01T16:48:02Z

Oh I see there's some jshint stuff sprinkled around. It's not maintained. I'll land a linting change separately, probably standard.

bendrucker · 2018-03-01T17:15:05Z

Just pushed a big update that uses standard, my preferred tool. The conflicts shouldn't be too bad (just toss package.json). asi (automatic semicolon insertion) was on for jshint in some modules, standard just assumes it everywhere.

Hopefully this helps make style consistency less ambiguous/easier. I've also been kicking around the idea of spending some time each day touching up the tests as a way to re-familiarize myself with the project.

Keen on trying to resolve bugs like this and caching issues. I've never personally run into them which is probably a reflection of how I structure projects.

Some folks at my new job are starting to adopt pq so I should have some opportunities to focus on it more regularly.

…g required.

jwalton · 2018-03-01T18:14:48Z

There we go - I think that grabs all your changes, and it's rebased on master so it's all "standard"ized.

bendrucker · 2018-03-01T18:18:29Z

Nice! I should have time to have a look at this again today. Particularly keen to see what's failing with respect to non-relative packages.

jwalton · 2018-03-01T18:23:28Z

The problem is that the unit tests try to require global packages that don't actually exist. Before, this was fine, because we didn't resolve them until the cleanup step, and we just ignored the errors (and possibly polluted the cache a little tiny bit? But with modules that don't actually exist, so I suppose that's OK.)

Now that we resolve them ahead of time, though, so we hit the error earlier, and have to decide what to do. This fix just leaves them unchanged. It also means we'll try to clean them up from the cache now.

bendrucker

Looking good, appreciate all your efforts here! Made a few comments, will take another pass pre-merge and verify that the test fails without the source changes.

bendrucker · 2018-03-01T23:38:58Z

lib/proxyquire.js

+// "relative" paths (e.g. "./foo") but not non-relative modules (e.g. "react").
+// Also, if noPreserveCache(), then we won't throw if the module can't
+// be resolved.
+Proxyquire.prototype._resolveRelativePath = function (path, moduleFilename) {


_resolvePath, it may only be doing stuff with relative paths but it's responsible for parsing all stub paths.

Oh, good catch. :)

The comment here is now wildly incorrect, too. :)

bendrucker · 2018-03-01T23:41:34Z

lib/proxyquire.js

@@ -4,6 +4,7 @@
 var Module = require('module')
 var resolve = require('resolve')
 var dirname = require('path').dirname
+var pathResolve = require('path').resolve


Much as it'll be a bit disruptive I think we should just do var path = require('path') and rename other "path" names to be more descriptive. I'd rather someone see path.* especially for fns that aren't obviously named (resolve).

Yeah, I was thinking the same thing. I'll do this.

bendrucker · 2018-03-01T23:42:26Z

lib/proxyquire.js

-  if (hasOwnProperty.call(stubs, path)) {
-    var stub = stubs[path]
+  var resolvedPath = this._resolveRelativePath(path, module.filename)
+


no linebreak since this gets used into the if block immediately after

bendrucker · 2018-03-01T23:43:51Z

lib/proxyquire.js


    if (stub === null) {
      // Mimic the module-not-found exception thrown by node.js.
-      throw moduleNotFoundError(path)
+      throw moduleNotFoundError(resolvedPath)


Shouldn't this be the original user input? This is simulating the error message node would generate and that would be based on the require id the user passes, not the resolved path.

bendrucker · 2018-03-01T23:44:44Z

lib/proxyquire.js

-      })
-      var ids = [id].concat(stubIds.filter(Boolean))
-
+      var stubIds = Object.keys(stubs)


You can merge this line with the next now (Object.keys(stubs).filter(Boolean))

bendrucker · 2018-03-01T23:48:47Z

lib/proxyquire.js

+  } catch (err) {
+    // If this is not a relative path (e.g. "foo" as opposed to "./foo"), and
+    // we couldn't resolve it, then we just let the path through unchanged.
+    if (path[0] !== '.') {


Thoughts on using https://github.com/mattdesl/relative-require-regex here?

IMO an absolute path that couldn't be resolved should also pass through. External module also has the benefits of separate tests and docs.

Could do - it's not a very exciting module: https://github.com/mattdesl/relative-require-regex/blob/master/index.js :P I tend to lean towards not using dependencies if I can avoid them.

The only thing different between their version and mine is, they would count something that starts with a "/" as a relative path. We're only going to hit this case when the path doesn't exist, so it's already a super rare case. Really this is here to keep behavior consistent with what used to happen (and to make the unit tests pass, since they bring in global modules that don't exist).

If you have strong feelings about it one way or the other, though, I'm happy to go either way.

To me it comes down to whether you exactly reproduce node or mostly reproduce it in the ways we think matter. I'd rather err on the side of trying to replicate node behavior directly and that errs on the side of external packages since there are lots of cases to manage. I agree that this is exceptionally unlikely to matter but I'd rather build to spec.

We aren't really trying to reproduce node behavior here, though. Any path that starts with a "." or ".." is unsafe for us to let through, because it wasn't resolved, but it could resolve relative to some other source file (that's exactly the issue I raised in the first place). A file that starts with a "/", on the other hand, can only match one file on the disk, so it's safe to let through even if the file doesn't exist. We just want to make sure that anything we return from this function is a "unique" module name.

Oh... And if I change this to use relative-require-regex, two existing tests fail:

So these tests would need some rework... We could add the noPreserveCache() setting to these tests, so it falls into the second case, I suppose.

Cool. Keep it is as for now. I'll look to remove that path eventually.

bendrucker · 2018-03-02T22:23:09Z

Thank you again for your help here! I verified that the test fails in the absence of your changes (git checkout master -- lib/*). Waiting for Travis now just to be safe and then I'll push a new major. Since this is a bug and the major bump is just to be conservative/practical I don't think there's a need to mention the change in the readme.

jwalton · 2018-03-03T00:07:34Z

Thanks for merging! I would put a note somewhere. Maybe add a changelog.md? We had tests in our codebase that fail with this change, because someone was trying to proxyquire a file that wasn't being required but the file directly, but by some other file.

…

On Mar 2, 2018 17:23, "Ben Drucker" ***@***.***> wrote: Thank you again for your help here! I verified that the test fails in the absence of your changes (git checkout master -- lib/*). Waiting for Travis now just to be safe and then I'll push a new major. Since this is a bug and the major bump is just to be conservative/practical I don't think there's a need to mention the change in the readme. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#190 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABsF-0FlZ8cjb8r_PPYKjPvqSdw-CspUks5tacZOgaJpZM4SXvqi> .

gr2m · 2018-03-03T02:23:13Z

It would be great if you could put a note into the v2.0.0 release: https://github.com/thlorenz/proxyquire/releases/tag/v2.0.0

or create a changelog

Lots of folks will look for that :)
https://github.com/search?q=%22Update+proxyquire+to+the+latest+version%22&type=Issues&utf8=%E2%9C%93

if you like to automate releases (incl. the change logs) check out https://github.com/semantic-release/semantic-release ;)

bendrucker · 2018-03-03T02:42:17Z

Updated the release with a short note. Not a fan of changelogs for small projects with low volume since it creates an expectation of updates on every release versus asking users to actually look at the changes. Putting some bullets on GH releases that are noteworthy is fine though.

Previous code was based on an error in proxyquire: <2.0.0 thlorenz/proxyquire#190

jwalton commented Mar 1, 2018

View reviewed changes

bendrucker requested changes Mar 1, 2018

View reviewed changes

Jason Walton added 2 commits March 1, 2018 13:04

Make it so relative proxyquired paths are relative to the module bein…

5e5411a

…g required.

Fixes from code inspect.

cbceb8f

bendrucker reviewed Mar 1, 2018

View reviewed changes

More changes from code inspect.

64cfcc6

bendrucker merged commit c375628 into thlorenz:master Mar 2, 2018

dependabot-preview bot mentioned this pull request Mar 20, 2018

Bump proxyquire from 1.8.0 to 2.0.1 holvonix-misc/pullmaster#56

Closed

espy added a commit to hoodiehq/hoodie-server that referenced this pull request Apr 20, 2018

test: remove unneded stub for app-config

4afd63d

Previous code was based on an error in proxyquire: <2.0.0 thlorenz/proxyquire#190

espy mentioned this pull request Apr 20, 2018

Update dependencies to enable Greenkeeper 🌴 hoodiehq/hoodie-server#544

Open

Make it so relative proxyquired paths are relative to the module being required. #190

Make it so relative proxyquired paths are relative to the module being required. #190

Conversation

jwalton commented Mar 1, 2018 • edited by bendrucker Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendrucker 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

bendrucker commented Mar 1, 2018

bendrucker commented Mar 1, 2018

jwalton commented Mar 1, 2018

bendrucker commented Mar 1, 2018 • edited Loading

jwalton commented Mar 1, 2018

bendrucker 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

bendrucker Mar 1, 2018 • edited Loading

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

bendrucker Mar 2, 2018 • edited Loading

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

bendrucker commented Mar 2, 2018

jwalton commented Mar 3, 2018 via email

gr2m commented Mar 3, 2018

bendrucker commented Mar 3, 2018

jwalton commented Mar 1, 2018 •

edited by bendrucker

Loading

bendrucker commented Mar 1, 2018 •

edited

Loading

bendrucker Mar 1, 2018 •

edited

Loading

bendrucker Mar 2, 2018 •

edited

Loading