Failing test

[jscheid]

I was thinking of taking a shot at #2, but it would be nice if all tests would pass first.

$ git log --oneline -1
b09fbdc (HEAD -> master, tag: v1.4.0, origin/master, origin/HEAD) v1.4.0
$ git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
$ yarn run jest
yarn run v1.15.2
$ /Users/redacted/Library/Caches/Yarn/v4/npm-jest-23.6.0-ad5835e923ebf6e19e7a1d7529a432edfee7813d/node_modules/jest/.bin/jest
 FAIL  ./index.test.js
  Regular Plugin
    ✓ should correctly resolve a relative require (84ms)
    ✓ shouldn't prevent the 'extensions' option from working (8ms)
    ✓ shouldn't prevent the 'alias' option from working (4ms)
    ✕ shouldn't prevent the 'modules' option from working (115ms)

  ● Regular Plugin › shouldn't prevent the 'modules' option from working

    You cannot require a package ("file") that is not declared in your dependencies (via "/Users/redacted/src/pnp-webpack-plugin/")

      53 | 
      54 | function makeError(code, message, data = {}) {
    > 55 |   const error = new Error(message);
         |                 ^
      56 |   return Object.assign(error, {code, data});
      57 | }
      58 | 

      at makeError (.pnp.js:55:17)
      at Object.makeError [as resolveToUnqualified] (.pnp.js:5964:17)
      at resolveToUnqualified (index.js:92:26)
      at AsyncSeriesBailHook.eval [as callAsync] (eval at create (../../Library/Caches/Yarn/v4/npm-tapable-1.1.0-0d076a172e3d9ba088fd2272b2668fb8d194b78c/node_modules/tapable/lib/HookCodeFactory.js:32:10), <anonymous>:7:1)
      at Resolver.doResolve (../../Library/Caches/Yarn/v4/npm-enhanced-resolve-4.1.0-41c7e0bfdfe74ac1ffe1e57ad6a5c6c9f3742a7f/node_modules/enhanced-resolve/lib/Resolver.js:235:16)
      at resolver.getHook.tapAsync (../../Library/Caches/Yarn/v4/npm-enhanced-resolve-4.1.0-41c7e0bfdfe74ac1ffe1e57ad6a5c6c9f3742a7f/node_modules/enhanced-resolve/lib/TryNextPlugin.js:17:13)
      at AsyncSeriesBailHook.eval [as callAsync] (eval at create (../../Library/Caches/Yarn/v4/npm-tapable-1.1.0-0d076a172e3d9ba088fd2272b2668fb8d194b78c/node_modules/tapable/lib/HookCodeFactory.js:32:10), <anonymous>:7:1)
      at Resolver.doResolve (../../Library/Caches/Yarn/v4/npm-enhanced-resolve-4.1.0-41c7e0bfdfe74ac1ffe1e57ad6a5c6c9f3742a7f/node_modules/enhanced-resolve/lib/Resolver.js:235:16)
      at resolver.getHook.tapAsync (../../Library/Caches/Yarn/v4/npm-enhanced-resolve-4.1.0-41c7e0bfdfe74ac1ffe1e57ad6a5c6c9f3742a7f/node_modules/enhanced-resolve/lib/ModuleKindPlugin.js:19:13)
      at AsyncSeriesBailHook.eval [as callAsync] (eval at create (../../Library/Caches/Yarn/v4/npm-tapable-1.1.0-0d076a172e3d9ba088fd2272b2668fb8d194b78c/node_modules/tapable/lib/HookCodeFactory.js:32:10), <anonymous>:7:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 3 passed, 4 total
Snapshots:   0 total
Time:        0.73s, estimated 2s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
$ 

[arcanis]

This failing test is related to #3, which I think is a wontfix (I should remove the test). The modules field (just like NODE_PATH) simply doesn't make sense w/ PnP.

[jscheid]

Ah, good to know, thanks.

[jscheid]

@arcanis I might be missing something, but it appears the before-module hook is never invoked by the passing tests. I can add a throw in there and the same tests still pass. Isn't that the core of the plugin?

[arcanis]

The tests under their current form mostly are "it should not break feature X from webpack". Since Yarn itself is built using pnp-webpack-plugin (and thus acts as a de-facto integration test), I didn't take the time to write extensive tests that validate that it works fine in a PnP environment (it would be a good idea, though).

Of note, the current tests don't go through the before-module because they happen before in the resolution step. One additional test we should have is a version of "shouldn't prevent the 'extensions' option from working" that doesn't use relative paths, so that we can check that the extensions are properly resolved even when PnP is used to resolve a path.

[arcanis]

Of note: I'm currently in the process of proposing it for being a builtin plugin, in which case it would benefit from all the existing test infra (it also mocks a small PnP API so it's a bit more extensive that the current testsuite).

[jscheid]

Oh. In that case, it would be better to backport that PR to older enhanced-resolve branches, if only because it would allow replacing this terrible, terrible hack:

resolver._plugins.file = resolver._plugins.file.map(plugin => (
  plugin.toString().indexOf('"resolved symlink to "') < 0 ? plugin : (request, callback) => callback()
));

by something much nicer like https://github.com/webpack/enhanced-resolve/pull/168/files#diff-2d048cd0f0a4be9ba200a00089d3f3edR295

I'm afraid this turns out too deep a rabbit hole for me. I'll first try working around it by avoiding this plugin altogether - by using something like { loader: require.resolve('style-loader' } instead of "style-loader").

[arcanis]

I'm curious though - what's preventing you from upgrading to more recent Webpack releases? Wouldn't that be less work?

[jscheid]

I'm refactoring tests for a Webpack plugin (you might remember it) and I'd rather not drop compatibility with, and thus tests for, (at least) Webpack 3 just yet. I will, if this turns into a major ordeal, but I'm pretty close to a simple solution for keeping them around.