Adds support for PnP #168

arcanis · 2019-01-28T15:12:56Z

This PR adds a small PnP plugin, and the associated tests.

Fixes #162

jsf-clabot · 2019-01-28T15:12:58Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ arcanis
✅ sokra
❌ Maël Nison

Maël Nison seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

codecov · 2019-01-28T15:14:55Z

Codecov Report

Merging #168 into master will decrease coverage by <.01%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   92.98%   92.97%   -0.01%     
==========================================
  Files          32       33       +1     
  Lines        1140     1167      +27     
  Branches      262      269       +7     
==========================================
+ Hits         1060     1085      +25     
- Misses         80       82       +2

Impacted Files	Coverage Δ
lib/ResolverFactory.js	`98.06% <88.88%> (-0.58%)`	⬇️
lib/PnpPlugin.js	`89.47% <89.47%> (ø)`
lib/Resolver.js	`88.09% <0%> (+0.79%)`	⬆️

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 5c1495a...b24b62e. Read the comment docs.

codecov · 2019-01-28T15:14:55Z

Codecov Report

Merging #168 into master will increase coverage by 0.06%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   92.98%   93.04%   +0.06%     
==========================================
  Files          32       33       +1     
  Lines        1140     1165      +25     
  Branches      262      270       +8     
==========================================
+ Hits         1060     1084      +24     
- Misses         80       81       +1

Impacted Files	Coverage Δ
lib/ResolverFactory.js	`98.08% <90.9%> (-0.56%)`	⬇️
lib/PnpPlugin.js	`93.33% <93.33%> (ø)`
lib/Resolver.js	`88.09% <0%> (+0.79%)`	⬆️

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 5c1495a...912a6ef. Read the comment docs.

codecov · 2019-01-28T15:14:56Z

Codecov Report

Merging #168 into master will decrease coverage by <.01%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   92.98%   92.97%   -0.01%     
==========================================
  Files          32       33       +1     
  Lines        1140     1167      +27     
  Branches      262      269       +7     
==========================================
+ Hits         1060     1085      +25     
- Misses         80       82       +2

Impacted Files	Coverage Δ
lib/ResolverFactory.js	`98.06% <88.88%> (-0.58%)`	⬇️
lib/PnpPlugin.js	`89.47% <89.47%> (ø)`
lib/Resolver.js	`88.09% <0%> (+0.79%)`	⬆️

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 5c1495a...b24b62e. Read the comment docs.

webpack-bot · 2019-01-28T15:14:56Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

arcanis · 2019-01-28T15:21:40Z

One potential issue - in the case of the pnp-webpack-plugin package, users had to manually configure resolverLoader in order to resolve their loaders from the Webpack configuration rather than the files that make the imports (iirc, requestContext.context.issuer was the path of the file). Any hint as to how it could be implemented in a transparent way? Maybe by using a different issuer?

sokra · 2019-01-28T19:56:26Z

issuer shouldn't be used here anyway. It should better use request.path.

arcanis · 2019-01-28T21:39:08Z

What's the intended semantic difference between request.context.issuer and request.path, btw?

arcanis · 2019-01-31T16:42:16Z

Ping? 😃

alexander-akait · 2019-02-14T09:49:24Z

@arcanis Can you accept CLA?

arcanis · 2019-02-15T09:42:37Z

Done 👍

lib/PnpPlugin.js

sokra · 2019-03-25T19:09:22Z

lib/ResolverFactory.js

@@ -280,7 +290,8 @@ exports.createResolver = function(options) {
 		aliasFields.forEach(item => {
 			plugins.push(new AliasFieldPlugin("file", item, "resolve"));
 		});
-		if (symlinks) plugins.push(new SymlinkPlugin("file", "relative"));
+		if (symlinks && !pnpApi)


Why is this necessary?

Because ambiguities with require.resolve, whose return value is used both as input for readFile and require. This property means that the return value of require.resolve must both be a valid filesystem path, and a unique identifier for require call.

It doesn't cause issues in general cases, except when using peer dependencies - in this case you cannot uniquely identify a package via its location on the disk, because it might be instantiated multiple times in the dependency tree (each time with a different set of dependencies). In order to solve this, Yarn generates "virtual packages" for all packages that list peer dependencies, which effectively are symlinks to the cache. The symlink doesn't do anything, except that it gives semantic information used in the resolution (for example webpack-cli-abc would be registered with webpack@1.0.0 in its dependencies, while webpack-cli-123 would be registered with webpack@2.0.0 - even though both would use the same symlink target of /cache/webpack-cli-1.0.0).

In order to work properly, this behavior requires the symlinks to be preserved, otherwise we lose the information previously stored.

I'll add a comment linking to this comment.

hmm I understand the issue, but this need to be solved in some other way. Currently this change alters symlink behavior for all paths even non-pnp paths (i. e. local paths). It's ok to disable symlinks, but only for pnp-resolved paths.

You can do this i. e. by adding a flag to the request (i. e. keepSymlinks) when resolved by the pnp plugin and handle this flag in the SymlinkPlugin.

webpack-bot · 2019-03-26T14:24:27Z

@arcanis Thanks for your update.

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

@sokra Please review the new changes.

arcanis · 2019-03-26T14:29:46Z

All clear @sokra 👍

arcanis · 2019-04-03T10:59:19Z

Ping?

alexander-akait · 2019-04-03T11:02:19Z

@arcanis sorry for delay, @sokra some busy, we review this asap

sokra · 2019-04-08T18:21:06Z

lib/ResolverFactory.js

@@ -280,7 +290,8 @@ exports.createResolver = function(options) {
 		aliasFields.forEach(item => {
 			plugins.push(new AliasFieldPlugin("file", item, "resolve"));
 		});
-		if (symlinks) plugins.push(new SymlinkPlugin("file", "relative"));
+		if (symlinks && !pnpApi)


hmm I understand the issue, but this need to be solved in some other way. Currently this change alters symlink behavior for all paths even non-pnp paths (i. e. local paths). It's ok to disable symlinks, but only for pnp-resolved paths.

You can do this i. e. by adding a flag to the request (i. e. keepSymlinks) when resolved by the pnp plugin and handle this flag in the SymlinkPlugin.

arcanis · 2019-04-08T21:46:59Z

Currently this change alters symlink behavior for all paths even non-pnp paths (i. e. local paths). It's ok to disable symlinks, but only for pnp-resolved paths.

@sokra I'm not convinced it's either possible or desirable - imagine a file making an import via a relative path, should the symlinks be resolved or not? Now imagine that the issuer is a PnP package, how does that affect your answer? Now imagine that it's not a package but a workspace, how does that change things? And what's the risk that the difference in environment would cause a package to work locally (where it would be "not-pnp") and break elsewhere (where it would be a package and thus "pnp")?

Wouldn't it be solved simply by documenting the behavior as expected? This can be revisited if we get bug reports, but I'd argue that it might be worth trying as initial implementation.

If still have concerns, what do you think of making the default of symlinks different based on whether PnP is enabled or not? It would became similar to this, allowing users to try to force the symlink resolution on:

// Resolve symlinks to their symlinked location
const symlinks =
	typeof options.symlinks !== "undefined" ? options.symlinks : !pnpApi;

arcanis · 2019-04-18T14:23:13Z

@sokra I went ahead and updated the PR with the solution I mentioned. Is it better?

arcanis · 2019-04-25T11:16:26Z

Ping? 😊

arcanis · 2019-05-07T12:01:14Z

Ping 😔

alexander-akait · 2019-05-07T12:25:24Z

@arcanis sorry for delay, @sokra on parental leave (sometimes children are born 😄 ), he return back on this week and this issue first in todo list

arcanis · 2019-05-07T12:42:57Z

Thanks a lot! 😊

arcanis · 2019-05-24T11:56:33Z

I understand that @sokra doesn't have the bandwidth, but is there someone else that could help drive this forward? It pains me to see that this issue has been opened for half a year now. It also makes it way harder for me to plan accordingly for the project I manage, which happens to be a critical Webpack dependency.

As I said before I'm more than willing to refactor or to explain the design, but I can't be expected to continue sending pings. Remember I'm also doing this because I think it would provide value to Webpack and Webpack's users too ...

alexander-akait · 2019-05-24T12:15:16Z

@arcanis in priority list, answer will be in near future (day - two day)

fixes #168 Co-authored-by: Maël Nison <nison.mael@gmail.com>

arcanis · 2019-07-04T19:25:00Z

Thanks for bearing with me! ☺️🎉

johnnyreilly · 2019-07-04T20:29:31Z

Awesome! What are the implications of this going in @arcanis? Does this mean pnp-webpack-plugin doesn't need to exist anymore?

arcanis · 2019-07-05T09:02:11Z

@johnnyreilly It will still be needed for people using TypeScript, to provide the hooks it contains - although maybe we could just support it by default if you're ok with it, since it's very little code 😃

johnnyreilly · 2019-07-05T14:27:46Z

maybe we could just support it by default if you're ok with it, since it's very little code 😃

Make it happen! 😁

Adds support for PnP

b24b62e

webpack-bot added the PR: unreviewed label Jan 28, 2019

arcanis mentioned this pull request Jan 28, 2019

Builtin pnp resolver #162

Closed

webpack-bot added the PR: CI-ok label Jan 28, 2019

alexander-akait requested a review from sokra January 29, 2019 09:10

sokra requested changes Mar 25, 2019

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Mar 25, 2019

Uses requestContext.path over requestContext.context

1af3906

webpack-bot removed the PR: reviewed-changes-requested label Mar 26, 2019

webpack-bot added PR: review-outdated PR: CI-not-ok and removed PR: CI-ok labels Mar 26, 2019

Maël Nison added 2 commits March 26, 2019 14:27

Fixes linting

f5ce5a6

Adds a comment about symlinks

59a1c45

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Mar 26, 2019

arcanis mentioned this pull request Mar 28, 2019

Failing test arcanis/pnp-webpack-plugin#9

Open

convert let to const

816e48a

sokra requested changes Apr 8, 2019

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Apr 8, 2019

Allows to enforce symlink resolution even w/ pnp

912a6ef

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Apr 18, 2019

arcanis mentioned this pull request Apr 20, 2019

Adds the configuration for PnP/Typescript facebook/create-react-app#6856

Merged

sokra added a commit that referenced this pull request Jul 4, 2019

add support for Yarn PnP

b765dbf

fixes #168 Co-authored-by: Maël Nison <nison.mael@gmail.com>

sokra closed this in c6b3399 Jul 4, 2019

arcanis mentioned this pull request Sep 15, 2019

TS / PnP compatibility vercel/next.js#8744

Merged

mjameswh mentioned this pull request Mar 19, 2021

SymlinkPlugin's behavior has changed since Webpack 5 #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for PnP #168

Adds support for PnP #168

arcanis commented Jan 28, 2019

jsf-clabot commented Jan 28, 2019 •

edited

Loading

codecov bot commented Jan 28, 2019

codecov bot commented Jan 28, 2019 •

edited

Loading

codecov bot commented Jan 28, 2019

webpack-bot commented Jan 28, 2019

arcanis commented Jan 28, 2019

sokra commented Jan 28, 2019

arcanis commented Jan 28, 2019

arcanis commented Jan 31, 2019

alexander-akait commented Feb 14, 2019 •

edited

Loading

arcanis commented Feb 15, 2019

sokra Mar 25, 2019

arcanis Mar 25, 2019

sokra Apr 8, 2019

webpack-bot commented Mar 26, 2019

arcanis commented Mar 26, 2019

arcanis commented Apr 3, 2019

alexander-akait commented Apr 3, 2019

sokra Apr 8, 2019

arcanis commented Apr 8, 2019 •

edited

Loading

arcanis commented Apr 18, 2019

arcanis commented Apr 25, 2019

arcanis commented May 7, 2019

alexander-akait commented May 7, 2019

arcanis commented May 7, 2019

arcanis commented May 24, 2019 •

edited

Loading

alexander-akait commented May 24, 2019

arcanis commented Jul 4, 2019

johnnyreilly commented Jul 4, 2019

arcanis commented Jul 5, 2019

johnnyreilly commented Jul 5, 2019

Adds support for PnP #168

Adds support for PnP #168

Conversation

arcanis commented Jan 28, 2019

jsf-clabot commented Jan 28, 2019 • edited Loading

codecov bot commented Jan 28, 2019

Codecov Report

codecov bot commented Jan 28, 2019 • edited Loading

Codecov Report

codecov bot commented Jan 28, 2019

Codecov Report

webpack-bot commented Jan 28, 2019

arcanis commented Jan 28, 2019

sokra commented Jan 28, 2019

arcanis commented Jan 28, 2019

arcanis commented Jan 31, 2019

alexander-akait commented Feb 14, 2019 • edited Loading

arcanis commented Feb 15, 2019

sokra Mar 25, 2019

Choose a reason for hiding this comment

arcanis Mar 25, 2019

Choose a reason for hiding this comment

sokra Apr 8, 2019

Choose a reason for hiding this comment

webpack-bot commented Mar 26, 2019

arcanis commented Mar 26, 2019

arcanis commented Apr 3, 2019

alexander-akait commented Apr 3, 2019

sokra Apr 8, 2019

Choose a reason for hiding this comment

arcanis commented Apr 8, 2019 • edited Loading

arcanis commented Apr 18, 2019

arcanis commented Apr 25, 2019

arcanis commented May 7, 2019

alexander-akait commented May 7, 2019

arcanis commented May 7, 2019

arcanis commented May 24, 2019 • edited Loading

alexander-akait commented May 24, 2019

arcanis commented Jul 4, 2019

johnnyreilly commented Jul 4, 2019

arcanis commented Jul 5, 2019

johnnyreilly commented Jul 5, 2019

jsf-clabot commented Jan 28, 2019 •

edited

Loading

codecov bot commented Jan 28, 2019 •

edited

Loading

alexander-akait commented Feb 14, 2019 •

edited

Loading

arcanis commented Apr 8, 2019 •

edited

Loading

arcanis commented May 24, 2019 •

edited

Loading