Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ResolverFactory.js #286

Closed
wants to merge 1 commit into from
Closed

Conversation

dlredden
Copy link

Issue #263 is preventing my company from upgrading to Webpack v5. This was the proposed fix that worked well for us so I created a PR.

Issue webpack#263 is preventing my company from upgrading to Webpack v5. This was the proposed fix that worked well for us so I created a PR.
@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

Merging #286 (2e296b8) into master (ff16fc2) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #286   +/-   ##
=======================================
  Coverage   94.93%   94.93%           
=======================================
  Files          39       39           
  Lines        1579     1579           
=======================================
  Hits         1499     1499           
  Misses         80       80           
Impacted Files Coverage Δ
lib/ResolverFactory.js 97.09% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff16fc2...2e296b8. Read the comment docs.

@alexander-akait
Copy link
Member

Please provide example how we can reproduce the problem

@ferdinando-ferreira
Copy link

Please provide example how we can reproduce the problem

@alexander-akait : https://github.com/ferdinando-ferreira/enhanced-resolve-pnpapi-bug

git clone https://github.com/ferdinando-ferreira/enhanced-resolve-pnpapi-bug.git enhanced-resolve-pnpapi-bug
cd enhanced-resolve-pnpapi-bug
npm install
npm run build

@alexander-akait
Copy link
Member

@ferdinando-ferreira Why do you bundle webpack? You should ignore this module (resolve.fallback.pnpapi: false) when you bundle webpack for browser, because browsers doesn't support pnp

@ferdinando-ferreira
Copy link

Why do you bundle webpack?

@alexander-akait : To provide the minimal usecase requested.

Regardless of usefulness it provides the exact error case reported:

require("pnpapi")

when it should be

require("./PnpPlugin").PnpApiImpl

because there is no pnpapi package.

Would you prefer a "realistic" use case that would hit the same error?

@alexander-akait
Copy link
Member

alexander-akait commented Mar 25, 2021

Please read https://yarnpkg.com/advanced/pnpapi, require("pnpapi") !== require("./PnpPlugin").PnpApiImpl

@ferdinando-ferreira
Copy link

ferdinando-ferreira commented Mar 25, 2021

Please read https://yarnpkg.com/advanced/pnpapi, require("pnpapi") !== require("./PnpPlugin").PnpApiImpl

You are correct! The root cause of the error must be then the original issue submitter accidentally bundling something with a require to webpack (or other package depending on enhanced-resolve) with target: web.

@dlredden, @dstampher: any chance that's the case?

@alexander-akait
Copy link
Member

Yep, in this case resolve.fallback.pnpapi: false should solve the problem

@sokra
Copy link
Member

sokra commented Apr 19, 2021

If you want to use PnP with the compiled bundle, it's not resolve.fallback but externals instead:

externals: {
  pnpapi: true
}

sokra added a commit to webpack/webpack that referenced this pull request Apr 19, 2021
@dlredden dlredden deleted the patch-1 branch June 15, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants