[Bug] Accessing a missing peerDependency throws unlisted dependency error

[bgotink]

Describe the bug

Accessing a missing peer dependency from within a package leads to

Error: A package is trying to access another package without the second one being listed as a dependency of the first one

rather than the expected "module not found". Note that the package is in fact correctly listed as peer dependency.

This, in turn, leads to issues when packages (correctly) only catch "module not found" errors on optional (peer) dependencies, e.g. in

https://github.com/waysact/webpack-subresource-integrity/blob/ab1f5f9/index.js#L18-L25

To Reproduce

# setup empty repo with berry
yarn add webpack-subresource-integrity webpack@~4
yarn node -e 'require("webpack-subresource-integrity")'

bram:/private/tmp/berry-test $ yarn node -e 'require("webpack-subresource-integrity")'
                               
/private/tmp/berry-test/.yarn/virtual/webpack-subresource-integrity-virtual-fa8536a74ed9d56a6a5b6edfe52bbd2c49eae7e61c54a7282dde48f8c580d680/node_modules/webpack-subresource-integrity/index.js:23
    throw e;
    ^

Error: A package is trying to access another package without the second one being listed as a dependency of the first one

Required package: html-webpack-plugin (via "html-webpack-plugin")
Required by: webpack-subresource-integrity@virtual:d4a8200a055d0a559d4e2f8c86347d647b4c6f848b77e7e192fd716a7fd86e9e6ddd399fd1ed72078947940a10d55d13d32e330523252b20909b75b6f17ae1ad#npm:1.3.2 (via /private/tmp/berry-test/.yarn/virtual/webpack-subresource-integrity-virtual-fa8536a74ed9d56a6a5b6edfe52bbd2c49eae7e61c54a7282dde48f8c580d680/node_modules/webpack-subresource-integrity/index.js)

    at Object.makeError (/private/tmp/berry-test/.pnp.js:10899:19)
    at resolveToUnqualified (/private/tmp/berry-test/.pnp.js:12472:43)
    at resolveRequest (/private/tmp/berry-test/.pnp.js:12518:31)
    at Object.resolveRequest.maybeLog [as resolveRequest] (/private/tmp/berry-test/.pnp.js:12557:32)
    at Function.module_1.default._resolveFilename (/private/tmp/berry-test/.pnp.js:11994:37)
    at Function.module_1.default._load (/private/tmp/berry-test/.pnp.js:11915:45)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/private/tmp/berry-test/.yarn/virtual/webpack-subresource-integrity-virtual-fa8536a74ed9d56a6a5b6edfe52bbd2c49eae7e61c54a7282dde48f8c580d680/node_modules/webpack-subresource-integrity/index.js:20:23)
    at Module._compile (internal/modules/cjs/loader.js:701:30)

Environment if relevant (please complete the following information):

• OS: macOS
• Node version: 10
• Yarn version: latest version downloaded via yarn policies set-version v2 and build of master at 27c1137

[arcanis]

There's a bug in that the message is meant to be clearer than the one you got (PnP can throw semantic errors that detail why a package couldn't be loaded):

https://github.com/yarnpkg/berry/blob/master/packages/berry-pnp/sources/loader/makeApi.ts#L511

However, even once this bug is fixed, the code property will still be an issue with the package you listed. Can we submit a PR to their project to add support for our semantic error codes?

(in retrospect the custom error codes might have been a compatibility mistake, but now that things are as they are...)

[bgotink]

It's not just that the message is not clear. This is a missing peer dependency, not an undeclared one. As such, an error with code MISSING_PEER_DEPENDENCY makes a lot more sense to me than UNDECLARED_DEPENDENCY. In other words, the error in the second throw statement in the snippet below seems more appropriate to me than the fourth.

berry/packages/berry-pnp/sources/loader/makeApi.ts

Lines 511 to 540 in 0655aea

	if (dependencyReference === null) {
	if (issuerLocator.name === null) {
	throw makeError(
	`MISSING_PEER_DEPENDENCY`,
	`Something that got detected as your top-level application (because it doesn't seem to belong to any package) tried to access a peer dependency; this isn't allowed as the peer dependency cannot be provided by any parent package\n\nRequired package: ${dependencyName} (via "${request}")\nRequired by: ${issuer}\n`,
	{request, issuer, dependencyName},
	);
	} else {
	throw makeError(
	`MISSING_PEER_DEPENDENCY`,
	`A package is trying to access a peer dependency that should be provided by its direct ancestor but isn't\n\nRequired package: ${dependencyName} (via "${request}")\nRequired by: ${issuerLocator.name}@${issuerLocator.reference} (via ${issuer})\n`,
	{request, issuer, issuerLocator: Object.assign({}, issuerLocator), dependencyName},
	);
	}
	} else if (dependencyReference === undefined) {
	if (issuerLocator.name === null) {
	throw makeError(
	`UNDECLARED_DEPENDENCY`,
	`Something that got detected as your top-level application (because it doesn't seem to belong to any package) tried to access a package that is not declared in your dependencies\n\nRequired package: ${dependencyName} (via "${request}")\nRequired by: ${issuer}\n`,
	{request, issuer, dependencyName},
	);
	} else {
	const candidates = Array.from(issuerInformation.packageDependencies.keys());
	throw makeError(
	`UNDECLARED_DEPENDENCY`,
	`A package is trying to access another package without the second one being listed as a dependency of the first one\n\nRequired package: ${dependencyName} (via "${request}")\nRequired by: ${issuerLocator.name}@${issuerLocator.reference} (via ${issuer})\n`,
	{request, issuer, issuerLocator: Object.assign({}, issuerLocator), dependencyName, candidates},
	);
	}
	}

I can open a ticket/PR on their repo once it's clear what extra error code the if-check should ignore.

[arcanis]

Yes definitely - the problem is that the reference isn't stored at all when the peer dependency isn't met, rather than being set to null (which would go into the MISSING_PEER_DEPENDENCY branch).

I think the fix is just to prepopulate packageDependencies with a [depName, null] entry for each of the declared peer dependencies here. Then once the linker goes into attachInternalDependencies and attachExternalDependents they'll get replaced if they got provided - leaving only the missing ones to null.