feat(cli): exit with code 1 when remix build fails

[gmaliar]

When builds fail remix does not exit with an error code which makes it hard to catch build failures in CI environments

remix/packages/remix-dev/compiler.ts

Lines 307 to 314 in 197f570

	return await Promise.all([
	manifestPromise.then(() => browserBuildPromise),
	serverBuildPromise
	]);
	} catch (err) {
	options.onBuildFailure(err as Error);
	return [undefined, undefined];
	}

Example:

❯ yarn create remix
❯ cd ./my-remix-app
❯ echo -e "import doesntExists from \"doesnt-exists\";\ndoesntExists();\n$(cat app/routes/index.tsx)" > app/routes/index.tsx
❯ npx remix build

npx remix build
Building Remix app in production mode...
The path "doesnt-exists" is imported in app/routes/index.tsx but doesnt-exists is not listed in your package.json dependencies. Did you forget to install it?
 > node_modules/@web-std/file/tsconfig.json:6:24: warning: Non-relative path "packages/file/src/lib.js" is not allowed when "baseUrl" is not set (did you forget a leading "./"?)
    6 │       "@web-std/file": ["packages/file/src/lib.js"]
      ╵                         ~~~~~~~~~~~~~~~~~~~~~~~~~~

 > app/routes/index.tsx:1:25: error: Could not resolve "doesnt-exists" (mark it as external to exclude it from the bundle)
    1 │ import doesntExists from "doesnt-exists";
      ╵                          ~~~~~~~~~~~~~~~

Build failed with 1 error:
app/routes/index.tsx:1:25: error: Could not resolve "doesnt-exists" (mark it as external to exclude it from the bundle)
Built in 571ms

❯ echo $?
0

[gmaliar]

accidentally closed this by force pushing to my remix dev branch #1527

[ryanflorence]

looks like we intended to have this code pass onBuildFailure, and then we can process.exit(1) there instead of a try catch

[ryanflorence]

ah, also we use a defaultOnBuildFailure handler, I think the process.exit(1) needs to go there.

@mjackson can you help us understand the intent here? Right now it just does console.error in the default build failure handler, seems like that's also the spot we'd want to process.exit(1) as well.

[gmaliar]

I think that the default on build error is a great place to exit with an error code.
Would you like me to change?

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[gmaliar]

@ryanflorence we actually got it wrong.
@jacob-ebey already added error handling for the cli here:

remix/packages/remix-dev/cli.ts

Line 66 in f1eef65

.catch(handleError);

The only problem with it, is that the defaultBuildFailureHandler doesn't throw anything so the error just get swallowed and it doesn't exit for some use-cases.

Now there's another thing we got wrong, the default build failure handler is also used in watch, if it would process.exit(1) on watch, it's counterproductive to the watch intent, so it's not the correct spot to implement the exit procedure.

Instead, I've created a different onBuidlFailure that throw an error but still formats the failure as the default handler.
I propose an internal change in the compiler.ts file that extracts the formatting into a function formatBuildFailure that the defaultOnBuildFailure will use to format errors and also the different onBuildFailure can use when is invoked from the cli.

[gmaliar]

@ryanflorence @mjackson can we get this merged?

[gmaliar]

@mjackson @kentcdodds @ryanflorence this has been ready for a while, can we get this merged?

[kentcdodds]

This looks good to me. I'll ping Michael and Ryan about it.