Fix #33847: [Bug]: npm create storybook@latest fails in Alpine contain#33897
Open
danielalanbates wants to merge 1 commit intostorybookjs:nextfrom
Open
Fix #33847: [Bug]: npm create storybook@latest fails in Alpine contain#33897danielalanbates wants to merge 1 commit intostorybookjs:nextfrom
npm create storybook@latest fails in Alpine contain#33897danielalanbates wants to merge 1 commit intostorybookjs:nextfrom
Conversation
…ine compatibility The `detect-port` package shells out to `ps -p` to check process ownership of ports, which fails on Alpine Linux / BusyBox where `ps` does not support the `-p` flag. Replace it with a pure Node.js implementation using `net.createServer()` that works on all platforms without spawning subprocesses. Fixes storybookjs#33847
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Contributor
📝 WalkthroughWalkthroughThe changes replace an external port-detection library (detect-port) with a built-in Node.js net module implementation for discovering available TCP ports. A new public Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant detectFreePort
participant checkPort
participant net as net.createServer()
Caller->>detectFreePort: detectFreePort(port)
loop For each port attempt
detectFreePort->>checkPort: checkPort(port)
checkPort->>net: createServer() & listen()
alt Port available
net-->>checkPort: success, port bound
checkPort-->>detectFreePort: return bound port
detectFreePort-->>Caller: return port
else Port unavailable
net-->>checkPort: error/fail
checkPort-->>detectFreePort: port not available
detectFreePort->>checkPort: checkPort(0) [OS assigns]
checkPort->>net: createServer() & listen(0)
net-->>checkPort: OS-assigned port
checkPort-->>detectFreePort: return OS port
detectFreePort-->>Caller: return OS port
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches
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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
code/core/src/core-server/utils/server-address.ts (1)
43-46:server.address()cast is technically safe but skips the null/string branchWithin the
listencallback of a TCP serverserver.address()is alwaysnet.AddressInfo, so this won't fail at runtime. However, under strict TypeScript the type isstring | net.AddressInfo | null, and the cast hides that. A narrow guard makes the intent explicit and keeps strict-mode lint clean.♻️ Defensive narrow
server.listen(port, () => { - const { port: assignedPort } = server.address() as net.AddressInfo; - server.close(() => resolve(assignedPort)); + const addr = server.address(); + if (!addr || typeof addr === 'string') { + server.close(() => reject(new Error('Unexpected server address type'))); + return; + } + server.close(() => resolve(addr.port)); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/server-address.ts` around lines 43 - 46, The code currently casts server.address() to net.AddressInfo inside the server.listen callback which hides the string|null branches; change the listen callback in server.listen(...) to first call const addr = server.address() and narrow-check that addr !== null && typeof addr !== 'string' before accessing addr.port (the assignedPort variable) and resolving; if the guard fails, close the server and reject or resolve with a safe fallback so TypeScript strict checks no longer require the unsafe cast.code/core/src/core-server/utils/server-address.test.ts (1)
57-78: Missing test coverage fordetectFreePortexported function and EADDRINUSE fallbackThe exported
detectFreePortis not directly tested. The EADDRINUSE branch indetectFreePort(falling back tocheckPort(0)) and non-EADDRINUSE error propagation are both untested. TheexactPort: truepath ingetServerPortis also missing. These are the core new behaviours introduced by this PR.Suggested additions:
import { detectFreePort } from './server-address'; describe('detectFreePort', () => { const mockServer = { /* shared mock */ }; beforeEach(() => { vi.mocked(net.createServer).mockReturnValue(mockServer as unknown as net.Server); }); it('should return the requested port when it is free', async () => { // address() returns requested port mockServer.address.mockReturnValue({ port: 3000 }); const result = await detectFreePort(3000); expect(result).toBe(3000); }); it('should fall back to OS-assigned port on EADDRINUSE', async () => { vi.mocked(net.createServer) .mockReturnValueOnce(/* server that emits EADDRINUSE */ ...) .mockReturnValueOnce(/* server that resolves port 49152 */ ...); const result = await detectFreePort(3000); expect(result).toBe(49152); }); it('should propagate non-EADDRINUSE errors', async () => { // server emits EACCES await expect(detectFreePort(80)).rejects.toMatchObject({ code: 'EACCES' }); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/server-address.test.ts` around lines 57 - 78, Add tests that directly exercise detectFreePort and the exactPort:true path of getServerPort: mock net.createServer via vi.mocked(net.createServer) to return a mockServer implementing listen, on, address and close; write one test where mockServer.address() returns the requested port to assert detectFreePort(requestedPort) resolves to that port; write a second test where the first mockServer triggers an 'error' event with code 'EADDRINUSE' and a second mockServer returns an OS-assigned port (address().port) to assert detectFreePort falls back to checkPort(0) and resolves to that OS port; and write a third test where the mockServer emits a non-EADDRINUSE error (e.g., code 'EACCES') and assert detectFreePort rejects with that error; also add a test calling getServerPort with exactPort: true to ensure it returns the requested port when free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/core-server/utils/server-address.test.ts`:
- Line 14: The test currently calls vi.mock('node:net') without enabling spying,
so change the mock invocation in server-address.test.ts to use
vi.mock('node:net', { spy: true }) so that original module functions can be
spied on; update the existing vi.mock('node:net') call (referenced in this file)
to include the { spy: true } option.
- Around line 63-73: Move the inline mock setup for net.createServer into a
beforeEach: create the mockServer object and call
vi.mocked(net.createServer).mockReturnValue(...) inside a beforeEach block (and
ensure beforeEach is imported from vitest), so tests reuse/reset the mock setup;
keep references to mockServer, net.createServer, vi.mocked, and any vi.fn()
handlers, and remove the same mock construction from the it body so the test
only asserts behavior.
In `@code/core/src/core-server/utils/server-address.ts`:
- Around line 54-62: The current detectFreePort implementation jumps to
checkPort(0) on the first EADDRINUSE, which loses the prior sequential-scan
behavior; update detectFreePort to, when a port argument is provided and
checkPort(port) fails with EADDRINUSE, try a bounded sequential scan (e.g., for
i from 1 to MAX_PORT_SCAN) calling checkPort(port + i) until one succeeds, only
calling checkPort(0) after the scan is exhausted; preserve rejecting other
errors immediately and make MAX_PORT_SCAN a clear constant so it can be
adjusted.
---
Nitpick comments:
In `@code/core/src/core-server/utils/server-address.test.ts`:
- Around line 57-78: Add tests that directly exercise detectFreePort and the
exactPort:true path of getServerPort: mock net.createServer via
vi.mocked(net.createServer) to return a mockServer implementing listen, on,
address and close; write one test where mockServer.address() returns the
requested port to assert detectFreePort(requestedPort) resolves to that port;
write a second test where the first mockServer triggers an 'error' event with
code 'EADDRINUSE' and a second mockServer returns an OS-assigned port
(address().port) to assert detectFreePort falls back to checkPort(0) and
resolves to that OS port; and write a third test where the mockServer emits a
non-EADDRINUSE error (e.g., code 'EACCES') and assert detectFreePort rejects
with that error; also add a test calling getServerPort with exactPort: true to
ensure it returns the requested port when free.
In `@code/core/src/core-server/utils/server-address.ts`:
- Around line 43-46: The code currently casts server.address() to
net.AddressInfo inside the server.listen callback which hides the string|null
branches; change the listen callback in server.listen(...) to first call const
addr = server.address() and narrow-check that addr !== null && typeof addr !==
'string' before accessing addr.port (the assignedPort variable) and resolving;
if the guard fails, close the server and reject or resolve with a safe fallback
so TypeScript strict checks no longer require the unsafe cast.
| }, | ||
| })); | ||
| vi.mock('detect-port'); | ||
| vi.mock('node:net'); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
vi.mock('node:net') is missing the spy: true option
♻️ Proposed fix
-vi.mock('node:net');
+vi.mock('node:net', { spy: true });Based on learnings: "Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vi.mock('node:net'); | |
| vi.mock('node:net', { spy: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/utils/server-address.test.ts` at line 14, The test
currently calls vi.mock('node:net') without enabling spying, so change the mock
invocation in server-address.test.ts to use vi.mock('node:net', { spy: true })
so that original module functions can be spied on; update the existing
vi.mock('node:net') call (referenced in this file) to include the { spy: true }
option.
Comment on lines
+63
to
+73
| const mockServer = { | ||
| unref: vi.fn(), | ||
| on: vi.fn(), | ||
| listen: vi.fn((_port: number, cb: () => void) => { | ||
| cb(); | ||
| return mockServer; | ||
| }), | ||
| address: vi.fn(() => ({ port: expectedFreePort })), | ||
| close: vi.fn((cb: () => void) => cb()), | ||
| }; | ||
| vi.mocked(net.createServer).mockReturnValue(mockServer as unknown as net.Server); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Mock setup belongs in beforeEach, not inside the it body
Both mockServer construction and vi.mocked(net.createServer).mockReturnValue(...) are placed inline in the test case. Per guidelines they should live in a beforeEach block, and beforeEach needs to be added to the vitest import.
♻️ Proposed fix
-import { describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it, vi } from 'vitest'; describe('getServerPort', () => {
const port = 3000;
+ const mockServer = {
+ unref: vi.fn(),
+ on: vi.fn(),
+ listen: vi.fn((_port: number, cb: () => void) => {
+ cb();
+ return mockServer;
+ }),
+ address: vi.fn(() => ({ port: 4000 })),
+ close: vi.fn((cb: () => void) => cb()),
+ };
+
+ beforeEach(() => {
+ vi.mocked(net.createServer).mockReturnValue(mockServer as unknown as net.Server);
+ });
+
it('should resolve with a free port', async () => {
const expectedFreePort = 4000;
-
- const mockServer = {
- unref: vi.fn(),
- on: vi.fn(),
- listen: vi.fn((_port: number, cb: () => void) => {
- cb();
- return mockServer;
- }),
- address: vi.fn(() => ({ port: expectedFreePort })),
- close: vi.fn((cb: () => void) => cb()),
- };
- vi.mocked(net.createServer).mockReturnValue(mockServer as unknown as net.Server);
const result = await getServerPort(port);
expect(result).toBe(expectedFreePort);
});
});Based on learnings: "Implement mock behaviors in beforeEach blocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/utils/server-address.test.ts` around lines 63 - 73,
Move the inline mock setup for net.createServer into a beforeEach: create the
mockServer object and call vi.mocked(net.createServer).mockReturnValue(...)
inside a beforeEach block (and ensure beforeEach is imported from vitest), so
tests reuse/reset the mock setup; keep references to mockServer,
net.createServer, vi.mocked, and any vi.fn() handlers, and remove the same mock
construction from the it body so the test only asserts behavior.
Comment on lines
+54
to
+62
| export function detectFreePort(port?: number): Promise<number> { | ||
| return checkPort(port || 0).catch((err) => { | ||
| if (err.code === 'EADDRINUSE') { | ||
| // Let the OS assign a free port | ||
| return checkPort(0); | ||
| } | ||
| return Promise.reject(err); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Behavior regression: OS-assigned fallback skips sequential port scanning
detect-port tried the requested port, then port+1, port+2, etc. until finding a free slot. The new implementation jumps directly to checkPort(0) (OS-assigned ephemeral) on the first EADDRINUSE. Users who start Storybook when port 3000 is busy will now see a random high-numbered port (e.g. 49152) instead of 3001. This is a user-visible regression.
Consider a bounded sequential scan before falling back to port 0:
🔧 Proposed sequential-scan fallback
-export function detectFreePort(port?: number): Promise<number> {
- return checkPort(port || 0).catch((err) => {
- if (err.code === 'EADDRINUSE') {
- // Let the OS assign a free port
- return checkPort(0);
- }
- return Promise.reject(err);
- });
-}
+export function detectFreePort(port?: number, maxTries = 10): Promise<number> {
+ const startPort = port || 0;
+ const tryPort = (p: number, remaining: number): Promise<number> =>
+ checkPort(p).catch((err) => {
+ if (err.code === 'EADDRINUSE') {
+ if (remaining > 0 && p > 0) {
+ return tryPort(p + 1, remaining - 1);
+ }
+ // Last resort: let the OS assign a free port
+ return checkPort(0);
+ }
+ return Promise.reject(err);
+ });
+ return tryPort(startPort, startPort > 0 ? maxTries : 0);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function detectFreePort(port?: number): Promise<number> { | |
| return checkPort(port || 0).catch((err) => { | |
| if (err.code === 'EADDRINUSE') { | |
| // Let the OS assign a free port | |
| return checkPort(0); | |
| } | |
| return Promise.reject(err); | |
| }); | |
| } | |
| export function detectFreePort(port?: number, maxTries = 10): Promise<number> { | |
| const startPort = port || 0; | |
| const tryPort = (p: number, remaining: number): Promise<number> => | |
| checkPort(p).catch((err) => { | |
| if (err.code === 'EADDRINUSE') { | |
| if (remaining > 0 && p > 0) { | |
| return tryPort(p + 1, remaining - 1); | |
| } | |
| // Last resort: let the OS assign a free port | |
| return checkPort(0); | |
| } | |
| return Promise.reject(err); | |
| }); | |
| return tryPort(startPort, startPort > 0 ? maxTries : 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/core-server/utils/server-address.ts` around lines 54 - 62, The
current detectFreePort implementation jumps to checkPort(0) on the first
EADDRINUSE, which loses the prior sequential-scan behavior; update
detectFreePort to, when a port argument is provided and checkPort(port) fails
with EADDRINUSE, try a bounded sequential scan (e.g., for i from 1 to
MAX_PORT_SCAN) calling checkPort(port + i) until one succeeds, only calling
checkPort(0) after the scan is exhausted; preserve rejecting other errors
immediately and make MAX_PORT_SCAN a clear constant so it can be adjusted.
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.
Fixes #33847
Summary
This PR fixes: [Bug]:
npm create storybook@latestfails in Alpine containersChanges
Testing
Please review the changes carefully. The fix was verified against the existing test suite.
This PR was created with the assistance of Claude Sonnet 4.6 by Anthropic | effort: low. Happy to make any adjustments!
Summary by CodeRabbit