Skip to content

NextJS: Verify Next runs on desired port & fail fast#24930

Merged
shilman merged 4 commits into
shilman/nextjs-server-mockfrom
shilman/nextjs-verify-port
Nov 27, 2023
Merged

NextJS: Verify Next runs on desired port & fail fast#24930
shilman merged 4 commits into
shilman/nextjs-server-mockfrom
shilman/nextjs-verify-port

Conversation

@shilman

@shilman shilman commented Nov 21, 2023

Copy link
Copy Markdown
Member

What I did

NextJS doesn't start on the desired port if that port is already being used by another process. However our Storybook setup assumes that it is being run on the specified port and will fail if not. This check exits the process if another port is being used.

This is a strawman for discussion and shouldn't be merged as is. The code actually gets called twice with two different PIDs since next-config gets evaluated twice. Consequently there is a race condition.

Manual testing

  1. Verify the dev server starts normally
  2. Start a second dev server while the first one is running and verify that the process exits early

🦋 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/core team 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>

@shilman shilman marked this pull request as draft November 21, 2023 08:08
const writePidFile = async (pid: number, { appDir }: { appDir: boolean }) => {
const routeDir = appDir ? join('app', '(sb)') : 'pages';
const storybookDir = join(process.cwd(), routeDir, 'storybookPreview');
const pidFile = join(storybookDir, 'pid.tsx');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this file name is right for the app dir?

Also this will overwrite the file every time we start, that's a bit icky. Can we do it with a middleware somehow?

@tmeasday tmeasday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good

@shilman shilman marked this pull request as ready for review November 27, 2023 12:29
@shilman shilman changed the title NextJS: Verify that NextJS is running on the desired port & fail fast NextJS: Verify that Next runs on desired port & fail fast Nov 27, 2023
@shilman shilman changed the title NextJS: Verify that Next runs on desired port & fail fast NextJS: Verify Next runs on desired port & fail fast Nov 27, 2023
@shilman shilman merged commit 3011241 into shilman/nextjs-server-mock Nov 27, 2023
@shilman shilman deleted the shilman/nextjs-verify-port branch November 27, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants