fix: incorrect package json handling#34515
Open
lino-levan wants to merge 1 commit intostorybookjs:nextfrom
Open
fix: incorrect package json handling#34515lino-levan wants to merge 1 commit intostorybookjs:nextfrom
lino-levan wants to merge 1 commit intostorybookjs:nextfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request modifies dependency resolution logic in a utility file by replacing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/common/utils/get-storybook-refs.ts (1)
25-35:⚠️ Potential issue | 🟠 MajorHard-coded
node_modulespath can miss valid dependencies in hoisted workspacesAt Line 25, resolution is pinned to
<packageDir>/node_modules/.... In hoisted setups, the dependency may exist only in an ancestornode_modules, so this returnsENOENTand the ref is silently skipped at Line 34.💡 Suggested fix (walk up parent directories before giving up)
+const findDependencyPackageJson = async (startDir: string, dep: string) => { + let current = startDir; + while (true) { + const candidate = join(current, 'node_modules', dep, 'package.json'); + try { + await readFile(candidate, { encoding: 'utf8' }); + return candidate; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + throw error; + } + } + + const parent = dirname(current); + if (parent === current) { + return undefined; + } + current = parent; + } +}; ... - const l = join(directory, 'node_modules', d, 'package.json'); + const l = await findDependencyPackageJson(directory, d); + if (!l) { + return undefined; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/utils/get-storybook-refs.ts` around lines 25 - 35, The code currently constructs a path using join(directory, 'node_modules', d, 'package.json') and returns undefined on ENOENT, which misses hoisted packages; modify the lookup in the function in get-storybook-refs.ts to walk up parent directories searching for node_modules/<dep>/package.json (e.g., loop using path.dirname until root or use a find-up helper) before giving up, then readFile and parse that first-found package.json (preserving the existing handling of storybook.url and returning { id: name, ...storybook, version }).
🧹 Nitpick comments (1)
code/core/src/common/utils/get-storybook-refs.ts (1)
22-42: Please add regression tests for the new lookup/error behaviorThis path is fragile and should be covered with focused tests for:
- package with exports mapping (original bug),
- missing
package.json(ENOENT), and- hoisted dependency resolution from ancestor directories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/common/utils/get-storybook-refs.ts` around lines 22 - 42, Add focused regression tests for get-storybook-refs.ts covering the three behaviors: (1) a package with an "exports" mapping/modern package.json that previously broke the JSON lookup — create a fixture package.json with exports and a storybook.url and assert the function returns the expected ref (refer to how deps is processed and readFile is used in the async map), (2) missing package.json ENOENT handling — simulate a dep folder without package.json (or mock readFile to throw an error with code 'ENOENT') and assert the function returns undefined and does not log a non-ENOENT warning, and (3) hoisted dependency resolution — create a nested directory structure with node_modules in an ancestor directory (or simulate lookup behavior) and assert the code can locate/resolve the hoisted package.json for the dep; implement tests using temp directories or fs mocks and call the module that contains the Promise.all over deps so you exercise the try/catch branch and verify the returned list contains undefined or the ref as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/common/utils/get-storybook-refs.ts`:
- Around line 25-35: The code currently constructs a path using join(directory,
'node_modules', d, 'package.json') and returns undefined on ENOENT, which misses
hoisted packages; modify the lookup in the function in get-storybook-refs.ts to
walk up parent directories searching for node_modules/<dep>/package.json (e.g.,
loop using path.dirname until root or use a find-up helper) before giving up,
then readFile and parse that first-found package.json (preserving the existing
handling of storybook.url and returning { id: name, ...storybook, version }).
---
Nitpick comments:
In `@code/core/src/common/utils/get-storybook-refs.ts`:
- Around line 22-42: Add focused regression tests for get-storybook-refs.ts
covering the three behaviors: (1) a package with an "exports" mapping/modern
package.json that previously broke the JSON lookup — create a fixture
package.json with exports and a storybook.url and assert the function returns
the expected ref (refer to how deps is processed and readFile is used in the
async map), (2) missing package.json ENOENT handling — simulate a dep folder
without package.json (or mock readFile to throw an error with code 'ENOENT') and
assert the function returns undefined and does not log a non-ENOENT warning, and
(3) hoisted dependency resolution — create a nested directory structure with
node_modules in an ancestor directory (or simulate lookup behavior) and assert
the code can locate/resolve the hoisted package.json for the dep; implement
tests using temp directories or fs mocks and call the module that contains the
Promise.all over deps so you exercise the try/catch branch and verify the
returned list contains undefined or the ref as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84c00454-bbd7-427f-bb38-6fc0fa91f9ef
📒 Files selected for processing (1)
code/core/src/common/utils/get-storybook-refs.ts
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
1 participant
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 I did
At work, I was getting this warning
on storybook startup and I thought it was weird (and I don't like the noise that it adds for no reason). I went to look into what storybook was doing and saw this code, then I went into refractor and saw that it had
"./*": "./lang/*.js"configured in the exports. The package.json was being remapped to./lang/package.json.jswhich obviously does not exist. At first I patched refractor because I thought it was a bug over there, then I realized that storybook should literally just not be using arequire.resolvehere. It's the wrong tool for the job, we should absolutely not be respecting exports.It looks like this bug existed from the beginning of autoconfig (#10753). Eventually someone hit the same issue as me and patched it, but incorrectly, adding the try catch and the warning that I got annoyed at today (#11023). A follow up changed the dependency emulating
require.resolvebut again missed the logic issue (#31338).I'm running this patch in prod at work. Thought it made sense to upstream it or the good of open source. Cheers.
Checklist for Contributors
Testing
This change is completely untested, to be honest. We don't use this feature at work so I can't pretend to say that I know the code works. It probably should, it's a simple change, but I don't have enough work time to learn storybook testing infra to write proper tests here. Hopefully this inspires some movement on this issue. Feel free to close if that's unacceptable.
The changes in this PR are covered in the following automated tests:
Manual testing
Untested, don't have time to test, sorry, code diff is simple though.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit