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

fix(compartment-mapper): Take only first matching tag of package exports #2275

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented May 8, 2024

Closes: #2276

Description

If a package.json "exports" is a list of ordered constraints, Endo’s Compartment Mapper currently uses all of the accepted entries and not just the first. This causes subsequent entries to override the first if they produce matching paths. I believe the intended behavior is to use the first that matches.

Security Considerations

This amounts to a difference in behavior between Endo and Node in the treatment of Package Exports.

Scaling Considerations

None.

Documentation Considerations

Fixing this issue will bring Endo into accord with Node.js’s somewhat light documentation on this feature.

Testing Considerations

Existing snapshot tests cover the behavior and must be updated to reflect the correct interpretation.

Compatibility Considerations

This will improve ecosystem compatibility. No known bundles depend on the existing behavior, but we cannot rule out the possibility.

There is a possibility that this change will break existing bundles that depend on the current erroneous behavior, for example preferring default over an endo tag. I am considering this a bugfix and not a breaking change since it is a bug to rely on the current behavior.

Upgrade Considerations

Even if this behavior might break the bundling process for existing contracts, it will not invalidate existing bundles. This change should not break upgrades but may create a speed bump for upgrading tool dependencies.

@kriskowal
Copy link
Member Author

@naugtur I’m interested in your take on whether this fix or the breaking tests are invalid.

@naugtur
Copy link
Member

naugtur commented May 8, 2024

This might surface incompatibilities we didn't discover earlier if they were overshadowed by later entries but that would be the only way it could break anyone.

I could argue it should be a break instead of a return.

I'm not sure whether this or the previous state was the desired behavior - if we have entries for import and Endo, regardless of order, we should probably prefer endo if someone has put in the effort to supply it.

So maybe tags need to be an ordered list that we check in order and pick one based on that.

@kriskowal kriskowal force-pushed the kriskowal-first-matching-tag branch from 1ba04e9 to 6764d03 Compare May 8, 2024 21:25
@kriskowal
Copy link
Member Author

Spot checking the snapshot tests, I’m convinced that the new behavior is consistent with Node.js’s design. I’m posting an update with updated snapshots.

@kriskowal kriskowal force-pushed the kriskowal-first-matching-tag branch from 6764d03 to 24ca4ac Compare May 8, 2024 21:32
@kriskowal kriskowal marked this pull request as ready for review May 8, 2024 21:32
@kriskowal kriskowal force-pushed the kriskowal-first-matching-tag branch from 24ca4ac to 4be16fc Compare May 8, 2024 21:41
@naugtur
Copy link
Member

naugtur commented May 8, 2024

I'll think if I can come up with counterexamples

@kriskowal kriskowal requested a review from naugtur May 8, 2024 23:57
@kriskowal kriskowal force-pushed the kriskowal-first-matching-tag branch 2 times, most recently from 78bc0f3 to efdbaa1 Compare May 9, 2024 18:39
@kriskowal kriskowal force-pushed the kriskowal-first-matching-tag branch from efdbaa1 to 5430a86 Compare May 10, 2024 00:16
Copy link
Member

@naugtur naugtur left a comment

Choose a reason for hiding this comment

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

This is an improvement.
I didn't come up with examples where this change would not be desirable.

One concern left is both old and new implementation is not prioritizing the endo-specific entry if there is one. We might want to have that. Worth debating later.

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 take first matching of conditional exports
2 participants