CLI: Fix get versions utility for NPM #29577
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| it('with constraint it throws an error if command output is not a valid JSON', async () => { | ||
| vi.spyOn(npmProxy, 'executeCommand').mockResolvedValueOnce('NOT A JSON'); | ||
|
|
||
| await expect(npmProxy.latestVersion('@storybook/core')).rejects.toThrow(); | ||
| await expect(npmProxy.latestVersion('@storybook/core', '5.X')).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
style: test description should clarify that this only applies when using version constraints, as unconstrained version queries now expect plain strings
There was a problem hiding this comment.
The updated test description explicitly focuses the scope to just when using version constraints to satisfy the fact that this test previously existed. I'm not sure why this test is even here when it's making an assumption on how an external system will fail and forcing it to fail by intentionally mocking a failing response from the external system. An integration test that actually confirms that the expected result and format from calling the external system is true would be a better test.
| it('with constraint it throws an error if command output is not a valid JSON', async () => { | ||
| vi.spyOn(pnpmProxy, 'executeCommand').mockResolvedValueOnce('NOT A JSON'); | ||
|
|
||
| await expect(pnpmProxy.latestVersion('@storybook/core')).rejects.toThrow(); | ||
| await expect(pnpmProxy.latestVersion('@storybook/core', '5.X')).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
logic: Test case should also verify behavior without constraint since both paths can throw JSON parse errors
There was a problem hiding this comment.
The behavior without constraint cannot be tested because it does not throw an error nor does it attempt to validate the correctness of the command output.
johnrcui
force-pushed
the
fix-get-versions-utility
branch
from
b5d9e82 to
170c43a
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 170c43a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
yannbf
changed the title
Closes #26553
What I did
This PR patches a core utility function used to retrieve package versions with npm or pnpm and affects everyone who tries to run
npx storybook@latest initusing npm or pnpm as a package manager.The command used to retrieve the latest package version
npm info <package> version --jsonon recent versions of npm no longer return a valid JSON response (eg."8.4.2"the semantic version wrapped in double quotes), but instead just returns an unquoted string. The same is the case with pnpm and also affects those using bun since the underlying call to get versions uses an npm command.Without having to figure out what p/npm version's this behavior changed and needing to keep track of it, the following PR resorts to just using
npm info <package> version(without the--jsonflag) as this behavior is consistent across all versions of p/npm.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR fixes version retrieval functionality in Storybook's package manager proxies to handle changes in npm/pnpm version output format.
runGetVersionsin NPM/PNPM/BUN proxies to handle unquoted version strings from newer package managers--jsonflag when fetching single version information since newer versions return plain stringsnpx storybook@latest initfrom working with npm/pnpm package managers