Skip to content
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

[legacy-framework] Fix to not build custom server during blitz start #2408

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

mabadir
Copy link
Collaborator

@mabadir mabadir commented May 28, 2021

Closes: blitz-js/legacy-framework#267

What are the changes and their implications?

  • Building Custom Server during Blitz Build
  • startCustomServer function, now doesn't build by default.
  • Introduce a separate buildCustomServer fn

Feature Checklist

Copy link
Member

@flybayer flybayer left a comment

Choose a reason for hiding this comment

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

Not sure if you are done? Mark as not-draft if you are done.

However, this doesn't fully address the original issue. The custom server is still being built on blitz start. I think we should separate the function into buildCustomServer and startCustomServer.

packages/server/src/next-utils.ts Outdated Show resolved Hide resolved
packages/server/src/build.ts Outdated Show resolved Hide resolved
@mabadir
Copy link
Collaborator Author

mabadir commented Jun 7, 2021

Yes sorry, I didn't fully understand the main concern from the original issue. I will review this PR today and will push few updates. Sorry about that.

@mabadir mabadir force-pushed the mabadir/custom-server-blitz-build branch from a20faae to cabca46 Compare June 7, 2021 23:40
@mabadir mabadir marked this pull request as ready for review June 7, 2021 23:41
Comment on lines 320 to 341
// eslint-disable-next-line @typescript-eslint/no-floating-promises
esbuild.build(esbuildOptions).then(() => {
spawnServer()
})
spawnServer()
Copy link
Member

@flybayer flybayer Jun 8, 2021

Choose a reason for hiding this comment

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

Looking good! But this is a regression for blitz dev use case.

Here's the full list. Sorry, I should have put this earlier.

  • blitz build builds custom server
  • blitz start runs but does not build custom server
  • blitz dev should both build and run custom server (this is working correctly in existing code, but it's not on this PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think adding:
await buildCustomServer() to this line will solve it:

Copy link
Member

Choose a reason for hiding this comment

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

That handles initial build, but not watch. Did you test blitz dev?

I think we should conditionally do spawnServer() vs

esbuild.build(esbuildOptions).then(() => {
      spawnServer()
})

based on watch flag. Currently esbuildOptions in this function are not used and so hot reload doesn't work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I rushed this one in. This is what I am coming up with now.

export function startCustomServer(
    cwd: string,
    config: ServerConfig,
    {watch}: CustomServerOptions = {},
) {
  const serverBuildPath = getCustomServerBuildPath()

  let spawnEnv = getSpawnEnv(config)
  if (config.env === "prod") {
    spawnEnv = {...spawnEnv, NODE_ENV: "production"}
  }

  return new Promise<void>((res, rej) => {
    let process: ChildProcess

    const RESTART_CODE = 777777

    const spawnServer = () => {
      process = spawn("node", [serverBuildPath], {
        cwd,
        env: spawnEnv,
        stdio: "inherit",
      })
          .on("exit", (code: number) => {
            if (code === 0) {
              res()
            } else if (watch && code === RESTART_CODE) {
              spawnServer()
            } else {
              rej(`server.js failed with status code: ${code}`)
            }
          })
          .on("error", (err) => {
            console.error(err)
            rej(err)
          })
    }

    if (watch) {
      // Handle build & Starting server
      const esbuildOptions = getEsbuildOptions()
      esbuildOptions.watch = {
        onRebuild(error) {
          if (error) {
            log.error("Failed to re-build custom server")
          } else {
            log.newline()
            log.progress("Custom server changed - restarting...")
            log.newline()
            //@ts-ignore -- incorrect TS type from node
            process.exitCode = RESTART_CODE
            process.kill("SIGABRT")
          }
        },
      }
      // eslint-disable-next-line @typescript-eslint/no-floating-promises
      esbuild.build(esbuildOptions).then(() => {
        spawnServer()
      })
    } else {
      // No build required, Start server
      spawnServer()
    }
  })
}

@flybayer
Copy link
Member

flybayer commented Jun 8, 2021

yay!

@flybayer flybayer changed the title Build custom server during blitz build Fix to not build custom server during blitz start Jun 8, 2021
@flybayer flybayer merged commit 1ef378b into canary Jun 8, 2021
@flybayer flybayer deleted the mabadir/custom-server-blitz-build branch June 8, 2021 17:03
@blitzjs-bot
Copy link
Contributor

Added @mabadir contributions for test

@itsdillon itsdillon changed the title Fix to not build custom server during blitz start [legacy-framework] Fix to not build custom server during blitz start Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build config and custom server during blitz build
3 participants