fix: find the `pnpapi` the `issuer` belongs to

[merceyz]

What's the problem this PR addresses?

• In some cases enhanced-resolve is outside the control of the pnpapi and can't require it
• The issuer might not belong to the same pnpapi as enhanced-resolve
• The issuer might not belong to a pnpapi at all and should use the node_modules resolution instead

Fixes https://github.com/arcanis/pnp-webpack-plugin/issues/32 for Webpack 5

How did you fix it?

Use module.findPnpApi to locate the correct pnpapi for the issuer, if a pnpapi isn't found continue the normal resolution

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: merceyz / name: Kristoffer K. (e2211b7ea76691973646901956f6150bd6d51d5a, c53553ce6bcf4c66caf9f0469e3f2075a069c543, c040ad8f30b88cb5723a821deb9ee19ade53644a, 61ca304306435690915e711275511bbd60941599, 97369bc4c55906476d7ae23c8c349606c082a225, b35d48827d1ad5a4a09313ac5c2059276e5a3c0d, 9a9d94c4a453c0cd917973bf9763e6a03e116434, 0dec9ad2097f110a8f0c5a8347f1b8fd3fb1b61c, 1d139584359b79e5b7ec3b4dfa0cefd271fbbfd1)
• :white_check_mark: login: alexander-akait / name: Alexander Akait (bf7b68f2310aa3d9f1d40845b186419096c3661c)

[alexander-akait]

I think better to fix it on yarn side?

[jgornick]

I think better to fix it on yarn side?

@alexander-akait Can you elaborate on how it could be fixed on the yarn side? Thank you for your input!

[merceyz]

It's not really a bug on our side, require('pnpapi') is supposed to return the pnpapi that the issuer (enhanced-resolve in this case) belongs to so it's working as intended

[jgornick]

@sokra @alexander-akait ^^^ Bump 😄

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 52.38095% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 92.47%. Comparing base (a998c7d) to head (bf7b68f). Report is 8 commits behind head on main.

Files	Patch %	Lines
lib/ResolverFactory.js	20.00%	6 Missing and 2 partials :warning:
lib/PnpPlugin.js	90.00%	1 Missing :warning:
lib/util/module-browser.js	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   92.85%   92.47%   -0.38%     
==========================================
  Files          43       44       +1     
  Lines        2042     2061      +19     
  Branches      598      603       +5     
==========================================
+ Hits         1896     1906      +10     
- Misses        118      125       +7     
- Partials       28       30       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jgornick]

@alexander-akait @sokra Just wanted to check-in again if you could help provide any insight on how to approach a fix for this issue? Again, I'm more than willing to help fix it too.

I've tried digging into this to figure out another way to help fallback to alternatives, but with the plugin system, I was getting lost as to how the flow of resolution works and where one might be able to intersect and fallback. I've also reached out to @arcanis to help provide any insight, and I think we're all kind of stuck.

Anything would be greatly appreciated! Thank you!

[alexander-akait]

Let's wait @arcanis answer, I am not against these changes

[sokra]

How to reproduce a scenario where you have packages that are yarn managed and packages that are non-yarn managed?

[jgornick]

How to reproduce a scenario where you have packages that are yarn managed and packages that are non-yarn managed?

@sokra I'm sorry for not including that. Here's a link to a repo to reproduce that's using this patch as the fix: https://github.com/jgornick/yarn-webpack-node-modules-to-pnp

[jgornick]

How to reproduce a scenario where you have packages that are yarn managed and packages that are non-yarn managed?

@sokra I'm sorry for not including that. Here's a link to a repo to reproduce that's using this patch as the fix: https://github.com/jgornick/yarn-webpack-node-modules-to-pnp

@sokra Please let me know if there's anything else I can provide or help with 😄

[jgornick]

@sokra @alexander-akait ^^^ Bump again 😄

Specifically looking for any information with how to resolve https://github.com/webpack/enhanced-resolve/pull/301#discussion_r701406073. I have a reproducible repository setup at https://github.com/jgornick/yarn-webpack-node-modules-to-pnp

Thank you!

[arcanis]

Let's wait @arcanis answer, I am not against these changes

Those changes seem reasonable to me (note that @merceyz is a core contributor on Yarn 🙂).

[jgornick]

I think the main issue here though is that enhanced-resolve doesn't have a way to fallback if this the PnP Plugin can't resolve a package. @merceyz is highlighting the issue here: https://github.com/webpack/enhanced-resolve/pull/301/files#diff-484df5a516d012258b8eb71e51625ef132b1c571ef3756b7c45433f4365e2e69R61

[alexander-akait]

@jgornick Can you fix lint?

[jgornick]

I'll try to look at adding tests sometime today.

Is it fair to say that out of the PnpPlugin, if it can't resolve a module that it should indeed call the callback without any arguments? Or is there a better way we can tell enhanced-resolve to try fallbacks?

cc @sokra @alexander-akait

[jgornick]

Bump ^^^ @sokra @alexander-akait 😄

[merceyz]

I would prefer if the FIXME was fixed before this is merged but as I said in https://github.com/webpack/enhanced-resolve/pull/301#discussion_r701406073 i'm not sure how and was hoping either of you did

[jgornick]

@sokra and @alexander-akait ... There should be a commit coming from @merceyz that should fix this issue so that if PnP is available, but still can't resolve a module, it'll fallback to trying node_modules again.

The only question I have is how can I satisfy the code coverage issues, especially in ResolverFactory? In order to test this, we'd have to mock a module so that the findPnpApi method is available. However, I don't see any other examples of that in the codebase, which makes me think it's an anti-pattern too 😄

One option I was thinking of is what if we made the findPnpApi an option coming into the resolver factory so that it could be provided too? Please let me know what you think so I can finish this PR up 😄

Thank you!

• Joe

[linux-foundation-easycla[bot]]

The committers are authorized under a signed CLA.

• :white_check_mark: Kristoffer K. (df1cab360bac366028ede2b30347152958570a52, 80a01c9c7c1799006fcbd26bd21532c669af5914, f525638917a74b75fcb76e4679c268e6ebe8f3e1)

[webpack-bot]

@merceyz Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@alexander-akait Please review the new changes.

[andreialecu]

Just hit an issue caused by this in the official TailwindCSS vscode extension.

It's currently broken with an error in enhanced-resolve about:

[Error - 10:42:06 PM] Tailwind CSS: Maximum call stack size exceeded
Error
    at new y (/Users/andreialecu/.vscode-insiders/extensions/bradlc.vscode-tailwindcss-0.7.1/dist/server/tailwindServer.js:56:286819)
    at /Users/andreialecu/.vscode-insiders/extensions/bradlc.vscode-tailwindcss-0.7.1/dist/server/tailwindServer.js:56:356021
    at Generator.next (<anonymous>)
    at o (/Users/andreialecu/.vscode-insiders/extensions/bradlc.vscode-tailwindcss-0.7.1/dist/server/tailwindServer.js:56:348919)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

I tracked it down to enhanced-resolve's PnP handling, and @merceyz pointed out to this PR.

More context in https://github.com/tailwindlabs/tailwindcss-intellisense/issues/432

Hoping this can be reviewed and merged soon. 🙏

[jgornick]

For what it's worth @sokra, we've been using @merceyz's fix since October and haven't run into any issues. Is there any chance this can make it in?

[jgornick]

Bump @sokra and @alexander-akait

My understanding is that we're only waiting on approvals? Any help or guidance to get this moving forward and merged into a release would be greatly appreciated!

[jgornick]

Bump @sokra and @alexander-akait

Any chance we can get guidance on next steps here?

[jgornick]

@sokra, @alexander-akait or @vankop Can you provide any sort of feedback for this PR? As mentioned above, we've been using @merceyz's branch build of enhanced-resolve for over a year and fixed the particular issue described in this PR.

Again, if there's anything I can do to help merge this in, I'm more than willing to help!

Please let me know either way.

[jgornick]

Bump @sokra, @alexander-akait or @vankop

Just wanting to know next steps for us here.

[jgornick]

Thank you @alexander-akait for resetting this PR. I'm taking a look at the failing lint/tests.

[alexander-akait]

Tests failed, can someone take a look and fix it?

[alexander-akait]

@merceyz @jgornick Friendly ping :smile:

[jgornick]

@alexander-akait Thank you! I’ll make some time this weekend.