-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Build: Update verdaccio logic #33282
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
base: next
Are you sure you want to change the base?
Changes from 3 commits
6c384e5
b1f4fba
dfac78c
26b42a2
b7447d8
4c11805
2adb630
5b2cb2c
2e32364
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,9 +74,13 @@ async function run() { | |
| // the package couldn't be imported, use npx to install and run it instead | ||
| } | ||
|
|
||
| const { default: packageJson } = await import(`storybook/package.json`, { | ||
| with: { type: 'json' }, | ||
| }); | ||
|
|
||
| const child = executeCommand({ | ||
| command: 'npx', | ||
| args: ['--yes', `${targetCli.pkg}@${versions[targetCli.pkg]}`, ...targetCli.args], | ||
| args: ['--yes', `${targetCli.pkg}@${packageJson.version}`, ...targetCli.args], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are you sure this is a safe change? it will change it from being the version of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the actual behavior will be the same, but I don't understand why the change is needed. |
||
| stdio: 'inherit', | ||
| }); | ||
| child.on('exit', (code) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,6 +380,10 @@ export abstract class JsPackageManager { | |
|
|
||
| const isLatestStableRelease = currentVersion === latestInRange; | ||
|
|
||
| if (latestInRange?.includes('-local.')) { | ||
| return `${packageName}@local`; | ||
| } | ||
|
Comment on lines
+383
to
+385
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The So I'm guessing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have tests for this code IIRC, we should ensure added behavior has tests in place. |
||
|
|
||
| if (isLatestStableRelease || !currentVersion) { | ||
| return `${packageName}@^${latestInRange}`; | ||
| } | ||
|
|
@@ -440,7 +444,8 @@ export abstract class JsPackageManager { | |
| current && (!constraint || satisfies(current, constraint)) && gt(current, latest) | ||
| ? current | ||
| : latest; | ||
| return `^${versionToUse}`; | ||
|
|
||
| return latest.includes('-local.') ? 'local' : `^${versionToUse}`; | ||
kasperpeulen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
Comment on lines
+447
to
449
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's find and examine the JsPackageManager.ts file
fd "JsPackageManager.ts" -t fRepository: storybookjs/storybook Length of output: 124 🏁 Script executed: # Get the file to understand context around lines 410-449
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '400,460p'Repository: storybookjs/storybook Length of output: 2645 🏁 Script executed: # Search for latestVersion method to understand its return type
rg -n "latestVersion" code/core/src/common/js-package-manager/JsPackageManager.ts -B 2 -A 5Repository: storybookjs/storybook Length of output: 3511 🏁 Script executed: # Search for 'local' dist-tag usage patterns in the codebase
rg -n "dist-tag.*local|--tag.*local|'local'" --type=ts --type=js code/ -B 2 -A 2Repository: storybookjs/storybook Length of output: 5887 🏁 Script executed: # Search for any Verdaccio configuration or publishing patterns
rg -n "verdaccio|dist-tag" --type=ts --type=js --type=json code/ -B 1 -A 1Repository: storybookjs/storybook Length of output: 47 🏁 Script executed: # Check for 'local' string usage in version context
rg -n "\-local\." --type=ts --type=js code/core/src/common/js-package-manager/ -B 3 -A 3Repository: storybookjs/storybook Length of output: 1257 🏁 Script executed: # Check line 383 context more carefully
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '370,390p'Repository: storybookjs/storybook Length of output: 978 🏁 Script executed: # Check if latestInRange could be null at line 383
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '360,395p'Repository: storybookjs/storybook Length of output: 1555 🏁 Script executed: # Check TypeScript configuration for strict mode
cat tsconfig.json | head -50Repository: storybookjs/storybook Length of output: 110 🏁 Script executed: # Check the exact control flow - does TypeScript narrow after throw?
cat -n code/core/src/common/js-package-manager/JsPackageManager.ts | sed -n '427,450p'Repository: storybookjs/storybook Length of output: 876 🏁 Script executed: # Verify TypeScript version and strict mode config
fd "tsconfig" -t f | head -5Repository: storybookjs/storybook Length of output: 235 🏁 Script executed: # Check main tsconfig location
find . -maxdepth 2 -name "tsconfig*.json" -type fRepository: storybookjs/storybook Length of output: 109 🏁 Script executed: # Check TypeScript strict mode configuration
cat code/tsconfig.json | grep -A 20 "compilerOptions"Repository: storybookjs/storybook Length of output: 693 Add JSDoc documentation for the local version behavior. The method properly guards against null values at lines 430–432 with an explicit throw that narrows the However, the special handling of local versions (checking for Add to the JSDoc comment for this method (around line 410): /**
* Return the latest version of the input package available on npmjs registry. If constraint are
* provided it return the latest version matching the constraints.
*
* For `@storybook/*` packages the latest version is retrieved from `cli/src/versions.json` file
* directly
+ *
+ * When the latest version contains a local pre-release tag (e.g., '8.0.0-local.1'), this method
+ * returns the string 'local' to indicate the package should be installed using the 'local' dist-tag.
*
* @param packageName The name of the package
* @param constraint A valid semver constraint, example: '1.x || >=2.5.0 || 5.0.0 - 7.2.3'
+ * @returns A semver version string prefixed with '^', or the string 'local' for local development versions
*/
🤖 Prompt for AI Agents |
||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use distinct names for registry wait steps.
Both wait steps are named "Wait for registry", which makes them indistinguishable in the GitHub Actions UI. Since they wait on different ports (6001 for the proxy and 6002 for Verdaccio), they should have distinct names.
Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents