expressots / expressots

A Typescript + Node.js lightweight framework for quick building scalable, easy to read and maintain, server-side applications 🐎
https://expresso-ts.com/
MIT License
1.66k stars 42 forks source link

expresso build is not returning proper exit code causing ci build to pass when it should be failing #506

Open edesdan opened 2 days ago

edesdan commented 2 days ago

Is there an existing issue for this?

Current behavior

See current output of expressots build command:

🐎 Expressots

Clean ./dist directory: [clean-dist] ✔️
(node:91265) ExperimentalWarning: CommonJS module /opt/homebrew/lib/node_modules/npm/node_modules/debug/src/node.js is loading ES Module /opt/homebrew/lib/node_modules/npm/node_modules/supports-color/index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
src/useCases/converter/html.converter.controller.ts:12:5 - error TS1435: Unknown keyword or identifier. Did you mean 'async'?

12     sync execute(@body() payload: HtmlToPdfRequestDTO): Promise<HtmlToPdfResponseDTO> {
       ~~~~

Found 1 error in src/useCases/converter/html.converter.controller.ts:12

Error executing command:: [Command failed with code 2] ❌

so the build is actually failing but:

echo $? is returning 0

And I think this is very bad since the CI build step is passing even if in reality build is failing:

Screenshot 2024-11-28 at 10 14 05

Screenshot 2024-11-28 at 10 51 57

Steps to reproduce

No response

Expected behavior

espressots should return proper exit codes on failures

Package version

2.16

Node.js version

22.x

In which operating systems have you tested?

Other

No response

rsaz commented 1 day ago

Hi @edesdan

I reproduced the issue, and you're right, this is actually a bug.

Environment:

sync is not a valid keyword in ts, when you run in dev or build locally the system will throw an error which is expected, as the keyword doesn't exist however as you well detected it doesn't terminate with the appropriate exit code. If you type echo $? you will see that the result below exit with 0.

Running dev locally

Compilation error in /home/me/repo/app-22/src/useCases/html.converter.controller.ts
[ERROR] 21:13:39 ⨯ Unable to compile TypeScript:
src/useCases/html.converter.controller.ts(7,5): error TS1435: Unknown keyword or identifier. Did you mean 'async'?

Building locally

src/useCases/html.converter.controller.ts:7:5 - error TS1435: Unknown keyword or identifier. Did you mean 'async'?

7     sync execute() {
      ~~~~

Found 1 error in src/useCases/html.converter.controller.ts:7

Error executing command:: [Command failed with code 2] ❌

In development mode, although it doesn't terminate with the appropriate exit code you can visually see the issue blocking the application to continue executing, that's why this issue "doesn't seems to happen" locally but does on ci. Ci relies exclusively relies on the exit code to determine step success.

Using this ci as example you can build the application successfully even with error because what we demonstrated above.

# This workflow will do a clean installation of node dependencies, cache/restore them, build the source code and run tests across different versions of node
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-nodejs

name: Node.js CI

on:
    push:
        branches: ["main"]
    pull_request:
        branches: ["main"]

jobs:
    build:
        runs-on: ubuntu-latest

        strategy:
            matrix:
                node-version: [22.x]
                # See supported Node.js release schedule at https://nodejs.org/en/about/releases/

        steps:
            - uses: actions/checkout@v4
            - name: Use Node.js ${{ matrix.node-version }}
              uses: actions/setup-node@v4
              with:
                  node-version: ${{ matrix.node-version }}
                  cache: "npm"
            - run: npm ci
            - run: npm run build
            - run: npm test

image

I saw on the CLI that we are not setting the exit code when it fails

export const runCommand = async ({
  command,
}: {
  command: string;
}): Promise<void> => {
  const { opinionated, entryPoint } = await Compiler.loadConfig();
  const outDir = getOutDir();

  try {
    switch (command) {
      case "dev":
        await execCmd(
          "tsx",
          opinionated
            ? await opinionatedConfig()
            : await nonOpinionatedConfig(),
        );
        break;
      case "build":
        if (!outDir) {
          printError(
            "Cannot build project. Please provide an outDir in tsconfig.build.json",
            "build-command",
          );
          process.exit(1);
        }
        await cleanDist(outDir);
        await compileTypescript();
        await copyFiles(outDir);
        break;
      case "prod":
        // ... production logic
        break;
      default:
        printError(`Unknown command: `, command);
        process.exit(1);
        break;
    }
  } catch (error: Error | any) {
    printError("Error executing command:", error.message);
    process.exit(1); // we could set 1 or the actual error code -> this is the line missing in the CLI that generates this misleading on CI
  }
};

I will add on the v3.0.0

edesdan commented 1 day ago

Hi, nice 👍 . Looking forward to see the fix as atm I cannot build and ship my app, thank you.