Add path mapping for app directory in package.json - #4099
Merged
Conversation
🦋 Changeset detectedLatest commit: 5bf2e1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Add path mapping for the app directory to improve module resolution.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the published npm package subpath exports to restore TypeScript type resolution for deep @primer/view-components/app/... imports after an exports map was introduced previously.
Changes:
- Add an
./app/*conditionalexportspattern that providestypes/import/defaulttargets for deepapp/**imports. - Add a patch changeset to publish the fix.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds an exports subpath for ./app/* intended to restore TypeScript resolution for deep app/** imports. |
| .changeset/sunny-trams-turn.md | Adds a patch changeset to release the package.json exports adjustment. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Comment on lines
+19
to
24
| "./app/*": { | ||
| "types": "./app/*.d.ts", | ||
| "import": "./app/*.js", | ||
| "default": "./app/*.js" | ||
| }, | ||
| "./*": "./*" |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
PR #4096 introduced an
exportsmap topackage.jsonfor the first time (to publish the new./classnamessubpath). Before that PR there was noexportsfield, so consumers could deep-import compiled component modules directly from the filesystem, e.g.:Once the
exportsmap was added, the only thing covering those deep paths is the catch-all"./*": "./*". Because that target is a plain string with notypescondition, TypeScript (undermoduleResolution: bundler/node16) maps the extensionless import to a literal path with no extension and can't locate the adjacent.d.ts. This broke type resolution for every@primer/view-components/app/...deep import across consumers (e.g.github-uihas ~27 such imports), surfacing as:This PR restores type resolution for deep
app/*imports by adding a conditional subpath that exposes thetypes,import, anddefaultconditions, placed before the catch-all so it takes precedence:Integration
No production runtime/behavior change — this only affects how the published package's subpaths resolve for type-checkers and bundlers. It restores the pre-#4096 ability for consumers to deep-import
app/*modules with full type information. Consumers (e.g.github-ui) require no code changes once a version including this fix is published.Risk Assessment
This is an additive
exportssubpath that only widens resolution back to pre-#4096 behavior. It is trivially reverted and verifiable viatsc --traceResolution.What approach did you choose and why?
I chose to add an explicit
./app/*conditional subpath rather than change the catch-all"./*"because:typescondition is required for TypeScript to map an extensionless deep import to its.d.ts; a bare string target ("./*": "./*") cannot express conditions../app/*keeps the change narrow and intentional (the publishedfileslist only shipsapp/**JS/d.ts plusstatic/*), while leaving the catch-all in place for any other static asset paths../app/*pattern is listed before./*so it wins forapp/...imports.I verified the fix against the installed package with a resolution trace:
Anything you want to highlight for special attention from reviewers?
.subpathtypes/import/requiretargets match the canonical entry points (I mirroredmain/module/typesfrom the existingpackage.json).app/paths through"./*"; those remain covered by the unchanged catch-all.patchchangeset framed as a regression fix for Addclassnames.jsSet export to the CSS build #4096.Accessibility
Merge checklist