feat(compartment-mapper): Import and export subpath patterns#3048
feat(compartment-mapper): Import and export subpath patterns#3048
Conversation
boneskull
left a comment
There was a problem hiding this comment.
No real concerns here. Not even nits, really
| * Package imports field for self-referencing subpath patterns. | ||
| * Keys must start with '#'. | ||
| */ | ||
| imports?: unknown; |
There was a problem hiding this comment.
(suggestion)
Steal this code for the type defs of exports and imports: https://github.com/sindresorhus/type-fest/blob/850b33c4dd292e0ff8cff039ee167d69be324fce/source/package-json.d.ts#L227-L248
| export type SubpathMapping = | ||
| | Array<[pattern: string, replacement: string]> | ||
| | Record<string, string>; |
| * The imports field provides self-referencing subpath patterns that | ||
| * can be used to create private internal mappings. | ||
| * | ||
| * @param {string} _name - the name of the package (unused, but kept for consistency) |
| } | ||
| for (const [key, value] of entries(imports)) { | ||
| // imports keys must start with '#' | ||
| if (!key.startsWith('#')) { |
| * @param {string} value | ||
| * @returns {boolean} | ||
| */ | ||
| const hasWildcard = (key, value) => key.includes('*') || value.includes('*'); |
There was a problem hiding this comment.
Isn't it also important to know which of these two values contains the wildcard(s)?
| * @param {string} segment | ||
| * @returns {boolean} | ||
| */ | ||
| const hasWildcard = segment => segment.includes(WILDCARD); |
There was a problem hiding this comment.
(could) Refactor and share with infer-exports.js
| return patternSegment === specifierSegment ? '' : null; | ||
| } | ||
|
|
||
| const wildcardIndex = patternSegment.indexOf(WILDCARD); |
There was a problem hiding this comment.
(could) if we're able to assert here, I'd probably want to assert that wildcardIndex is a non-negative integer
| `Globstar (**) patterns are not supported in pattern: "${pattern}"`, | ||
| ); | ||
| } | ||
| if (replacement.includes(GLOBSTAR)) { |
| t.is(node.value, null); | ||
| t.deepEqual(Object.keys(node.children), []); |
There was a problem hiding this comment.
(could) refactor to t.like(node, {value: null, children: []}) assuming it works the way I think it should
| }); | ||
|
|
||
| test('assertMatchingWildcardCount - throws for mismatched counts', t => { | ||
| const error = t.throws(() => assertMatchingWildcardCount('./*/a/*', './*')); |
There was a problem hiding this comment.
(complaining) t.throws() is weaksauce.
expect(
() => assertMatchingWildcardCount('./*/a/*', './*'),
'to throw',
{
message: expect.it(
'to match', /wildcard count mismatch/i,
'and', 'to match', /2/,
'and', 'to match', /1/
)
}
);(imperative) feel the power of BUPKIS
e72403c to
fca6c85
Compare
🦋 Changeset detectedLatest commit: e9d5c73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
df71dc8 to
122ea31
Compare
122ea31 to
8f8db1e
Compare
8f8db1e to
e9d5c73
Compare
boneskull
left a comment
There was a problem hiding this comment.
See suggested fix to the conditional in interpretImports. Once this is addressed I'll approve.
Otherwise, just a bunch of questions and suggestions.
| Node.js allows array values in exports as fallback lists, where each | ||
| entry is tried in order and the first file that exists on disk is used. | ||
| Pattern resolution in the compartment-mapper is a pure string operation | ||
| with no filesystem access. | ||
| Array fallbacks would require threading read powers through the pattern | ||
| matcher and changing the `SubpathReplacer` signature. | ||
| Node.js documentation discourages array fallbacks. | ||
| If a pattern value is an array, `interpretExports` yields all elements | ||
| as separate entries and the first match wins without fallback probing. |
There was a problem hiding this comment.
This seems to imply that we don't have 1:1 parity with Node.js, is this correct?
| * `*` matches any substring including `/` (Node.js semantics). | ||
| * Stripped during digest/archiving - expanded patterns become concrete module entries. | ||
| */ | ||
| patterns?: PatternDescriptor[]; |
There was a problem hiding this comment.
| patterns?: PatternDescriptor[]; | |
| patterns?: Array<PatternDescriptor>; |
I'm pretty sure we're almost exclusively using this form.
| * maps to an {@link Exports} value. | ||
| */ | ||
| export type ExportConditions = { | ||
| // eslint-disable-next-line no-use-before-define |
There was a problem hiding this comment.
We should disable this rule entirely in **/*.ts as ESLint doesn't understand type scoping. TS sources should use tseslint's implementation instead.
There was a problem hiding this comment.
This and the next two types might also want an attribution to type-fest, since it appears identical 😄
| path: never; | ||
| retained: never; | ||
| scopes: never; | ||
| patterns: never; |
There was a problem hiding this comment.
This appears redundant, since this field does not appear in CompartmentMapDescriptor. Its presence in a DigestedCompartmentDescriptor should cause an error, even without the above addition.
| patterns: never; |
| * When absent, the pattern resolves within the owning compartment. | ||
| * Set when propagating export patterns from a dependency package. | ||
| */ | ||
| compartment?: string; |
There was a problem hiding this comment.
I could have sworn we had exactOptionalPropertyTypes enabled. Maybe not.
| const wildcardIndex = pattern.indexOf('*'); | ||
| if (wildcardIndex === -1) { | ||
| exactEntries.set(pattern, { replacement: null, compartment }); | ||
| } else { | ||
| const prefix = pattern.slice(0, wildcardIndex); | ||
| const suffix = pattern.slice(wildcardIndex + 1); | ||
| wildcardEntries.push({ | ||
| prefix, | ||
| suffix, | ||
| replacementPrefix: null, | ||
| replacementSuffix: null, | ||
| compartment, | ||
| }); | ||
| } |
There was a problem hiding this comment.
(could) split out into function, and L138-L150 as well
|
|
||
| // Sort wildcard entries by prefix length descending for specificity. | ||
| // Node.js selects the pattern with the longest matching prefix. | ||
| wildcardEntries.sort((a, b) => b.prefix.length - a.prefix.length); |
There was a problem hiding this comment.
I'm gonna guess wildcardEntries doesn't get big enough to start worrying about incurring another loop over it.
| entry.prefix.length, | ||
| specifier.length - entry.suffix.length, | ||
| ); | ||
| const result = `${entry.replacementPrefix}${captured}${entry.replacementSuffix}`; |
There was a problem hiding this comment.
do we have some validation code that ensures there isn't anything fishy in exports/imports like /etc/pwd
| await t.throwsAsync( | ||
| () => import(new URL('app/multi-star-import.js', fixtureDir).href), | ||
| { | ||
| code: 'ERR_PACKAGE_PATH_NOT_EXPORTED', |
There was a problem hiding this comment.
does this actually work?? I didn't think AVA knew about code.
| - Prefer `/** @import */` over dynamic `import()` in JSDoc type annotations. | ||
| Use a top-level `/** @import {Foo} from 'bar' */` comment instead of inline | ||
| `{import('bar').Foo}` in `@param`, `@type`, or `@returns` tags. |
Closes: #2897
Description
Adds support for Node.js subpath pattern replacement in
package.jsonexportsandimportsfields to the compartment-mapper. This enables patterns like:{ "exports": { "./features/*.js": "./src/features/*.js" }, "imports": { "#internal/*.js": "./lib/*.js" } }The implementation matches Node.js semantics where
*is a string replacement token that matches any substring, including across/path separators. Patterns are matched by specificity: the pattern with the longest matching prefix wins, and exact entries always take precedence over wildcard patterns.Security Considerations
Pattern-matched module imports go through the same policy enforcement (
enforcePolicyByModule) as other module resolutions. Cross-compartment patterns (from dependency exports) resolve within the dependency's compartment, maintaining compartment isolation. The prefix/suffix string matching approach avoids regex-based matching, eliminating potential ReDoS concerns.Scaling Considerations
Pattern matching is O(p) where p is the number of wildcard patterns, sorted by prefix length for specificity. Exact entries are checked first via a
Maplookup. Results are cached via write-back tomoduleDescriptors, so each specifier is matched at most once.Documentation Considerations
Users can now use wildcard patterns in their
package.jsonexportsandimportsfields when using compartment-mapper. The patterns follow Node.js subpath pattern semantics:*matches any substring, including across/separatorsnullvalues) exclude subpaths from resolution**) patterns are silently ignored (matching Node.js behavior)Testing Considerations
/matching, specificity ordering,#-imports patterns, null-target exclusion, error cases (globstar rejection, wildcard count mismatch), and various input formats (tuples,PatternDescriptorarray, record object)loadLocation,importLocation,mapNodeModules,makeArchive,parseArchive,writeArchive,loadArchive,importArchive_subpath-patterns-assertions.js.--conditions=blue-moonto verify the correct branch is selected by both Node.js and the Compartment Mappercompartment-map.jsonin archive and asserts no compartment has apatternspropertyCompatibility Considerations
This is a purely additive feature. Existing packages without wildcard patterns are unaffected. The
patternsfield is only added to compartment descriptors when patterns exist (using conditional spread), so existing snapshot tests pass without modification.Upgrade Considerations
No breaking changes. Packages can start using wildcard patterns in
exports/importsimmediately after upgrading to a version with this feature.