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

feat(compartment-mapper): Relax package discovery #2642

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

kriskowal
Copy link
Member

Closes: #2636

Description

The compartment mapper is currently strict about the interpretation of peerDependencies and peerDependenciesMeta.optional as opposed to optionalDependencies and is technically correct, but that correctness and understanding is not evenly distributed among tools in the ecosystem.
If the Compartment Mapper instead tolerates the physical absence of expected packages, it defers an error that would be experienced under mapNodeModules to link/load since that might attempt to load a module from the missing package. This is not a particularly observable difference in behavior, but the error message will be slightly less informative.

In this change, we relax the behavior by default and provide a strict option to provide the more informative error and tolerate fewer ecosystem defficiencies.

Security Considerations

None.

Scaling Considerations

None.

Documentation Considerations

News updated.

We do not yet publish API documentation for Compartment Mapper, but the necessary annotations are present when these devices are more public.

Testing Considerations

Includes positive and negative tests for the strict behavior.

Compatibility Considerations

The nature of some errors during bundling may change. Bundling errors are not observed on Agoric’s chain, so that sensitivity should not cause problems.

Upgrade Considerations

None.

Base automatically changed from kriskowal-fix-dev-condition-implication to master November 21, 2024 20:16
@kriskowal kriskowal force-pushed the kriskowal-relax-discovery-2636 branch from f6e4355 to 1a74f6a Compare November 21, 2024 20:17
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gosh, this seems swell.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have any effect on packages that are optionally required but not otherwise mentioned in package.json?

An example from typescript:

try {
  require('source-map-support')
} catch {}

...where source-map-support is a dev dependency of typescript. So this require would not throw if a) someone has a local working copy of Microsoft/typescript or b) someone already has source-map-support in their node_modules (as they often do).

for (const [name, descriptor] of Object.entries(
commonDependencyDescriptors,
)) {
if (Object(descriptor) === descriptor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, Object(x) === x is new to me. what does it do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the terse way of checking typeof x === 'object' && x !== null. Object(x) returns x iff x is an object and not null, otherwise boxes primitives.

@kriskowal
Copy link
Member Author

kriskowal commented Nov 26, 2024

Does this have any effect on packages that are optionally required but not otherwise mentioned in package.json?

An example from typescript:

try {
  require('source-map-support')
} catch {}

...where source-map-support is a dev dependency of typescript. So this require would not throw if a) someone has a local working copy of Microsoft/typescript or b) someone already has source-map-support in their node_modules (as they often do).

No, this just tolerates the absence in .../node_modules of packages that are present in package.json, not accommodate the presence in .../node_modules of packages that are absent in package.json

@kriskowal kriskowal force-pushed the kriskowal-relax-discovery-2636 branch from 1a74f6a to ab885a2 Compare November 26, 2024 00:29
@kriskowal
Copy link
Member Author

@boneskull We could conceivably add an even sloppier mapNodeModules that recursively lists .../node_modules/{,@/}* and disregards the *dependencies in package.json entirely. With a link hook, we might even be able to make discovery of .../node_modules lazy. In any case, not in scope for this PR, but so you know: that is a possibility.

@kriskowal kriskowal enabled auto-merge November 26, 2024 00:34
@kriskowal kriskowal merged commit 768c9cb into master Nov 26, 2024
15 checks passed
@kriskowal kriskowal deleted the kriskowal-relax-discovery-2636 branch November 26, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compartment Mapper should gracefully include peerDependencies only if present
3 participants