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 arcanis/pnp-webpack-plugin#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]

The committers listed above are authorized under a signed CLA.

• ✅ login: merceyz / name: Kristoffer K. (e2211b7, c53553c, c040ad8, 61ca304, 97369bc, b35d488, 9a9d94c, 0dec9ad, 1d13958)
• ✅ login: alexander-akait / name: Alexander Akait (bf7b68f)

[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]

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 ⚠️
lib/PnpPlugin.js	90.00%	1 Missing ⚠️
lib/util/module-browser.js	0.00%	1 Missing ⚠️

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     

☔ View full report in Codecov by Sentry.
📢 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 #301 (comment). 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 #301 (comment) 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]

The committers are authorized under a signed CLA.

• ✅ Kristoffer K. (df1cab3, 80a01c9, f525638)

[jgornick]

@TheLarkInn and @alexander-akait Looks like this should be good to go.

[jgornick]

Just fix merge conflicts and we are good to go!

@TheLarkInn Just wanted to check-in to make sure you're good with the changes here?

[jgornick]

@TheLarkInn and @alexander-akait: Bump 😄

[jgornick]

Sorry for the weekly bump @alexander-akait and @TheLarkInn, but just trying to get this through as I think we're finally all ready to go 😄

[jgornick]

Weekly bump @TheLarkInn 😄

If we can get you to remove the changes requested flag, then we can at least get the workflows to run.

[jgornick]

Bump 😄

@alexander-akait, is there anything you can do to remove the blocker on here from @TheLarkInn?

[jgornick]

Just checking in again on this PR to see if we can get it merged?

cc @TheLarkInn @alexander-akait

[jgornick]

👋 Hey, it's me again 😄

Would love to get this merged so that we can stop referencing a custom build of enhanced-resolve.

cc @TheLarkInn @alexander-akait

[arcanis]

@TheLarkInn Is there anything still blocking the merge?

[alexander-akait]

Let's remove it from the top of file and put it inside processPnpApiOption

[alexander-akait]

Tested and added some feedback

[alexander-akait]

I'm a little concerned about performance, we need improve it

[webpack-bot]

@alexander-akait Thanks for your update.

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

@TheLarkInn Please review the new changes.

[jgornick]

Thank you @alexander-akait!