ThrowOnWarnings = true should make esbuild return non-zero exit code

[KKoukiou]

https://github.com/robinloeffel/esbuild-plugin-eslint/blob/main/source/index.ts#L49

In this line you need to return the 'errros' attribute so that esbuild will throw an exception. Additionally the throwOnErrors should be 'true' by default I feel.

[robinloeffel]

oh shoot, you are absolutely correct about the throwOnWarning && !throwOnError! that is now fixed. thank you once more!

however, i disagree with you, that throwOnError should be true as per default. that seems too opinionated to me, and may confuse new or existing users. "my build worked before adding this plugin, but with it, it doesn't? guess i'll remove the plugin again!"

[glensc]

by semver you can release 0.4.0 with breaking changes

[robinloeffel]

@glensc why? i haven't added new functionality since v0.3.0, only a strictly internal rewrite and bug fixes. the current behavior was always intended, i just didn't test my code enough. furthermore, breaking changes would require upping the major number of the version, not the minor number.

[glensc]

@robinloeffel I don't care about the change itself at all, I'm still trying to figure out the purpose of this plugin.

by semver every bump in 0.x series of the "minor" position is allowed to make breaking changes.

EDIT: the 2.0 spec says:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

so, any 0.y.z version can make breaking changes.

[KKoukiou]

oh shoot, you are absolutely correct about the throwOnWarning && !throwOnError! that is now fixed. thank you once more!

however, i disagree with you, that throwOnError should be true as per default. that seems too opinionated to me, and may confuse new or existing users. "my build worked before adding this plugin, but with it, it doesn't? guess i'll remove the plugin again!"

My take on this, is that someone who wants to use eslint wants also to enforce the errors at least. Otherwise they just depend on the developers being nice to take a look at the output and fix the issues. But up to you, since it's configurable I think it does not matter.

[KKoukiou]

@robinloeffel hey I just tested the latest release and unfortunately this is not fixed as I expected. THe problem is when throwOnError: true and throwOnWarning: true as well. This is the scenario when a user, wants esbuild to exit with non-zero code with either warnings or errors. It's honestly not a normal scenario, that someone wants to throw only on warning but not on errors. So this issue needs one more go. Can you please re-open it?

[KKoukiou]

Did not test it, but the code could be rewritten to something like to have the expected behavior imo.

      return {
        ...warnings.length > 0 && { [throwOnWarnings ? 'errors' : 'warnings']: warnings },
        ...errors.length > 0 && { [throwOnErrors ? 'errors' : 'warnings']: errors },
        }

[robinloeffel]

hey @KKoukiou

can you check out the repo and test it there? i've just tested the code there on another computer of mine, and it seems to work just fine. but to clarify:

• throwOnError === true && throwOnWarning !== true → make esbuild throw by returning an array of messages in errors
• throwOnError === true && throwOnWarning === true → make esbuild throw by returning an array of messages in errors, as well as additionally populate an array of messages in warnings
• throwOnError !==true && throwOnWarning === true → make esbuild throw by returning a dummy message in errors, as well as additionally populate an array of messages in warnings

the code you've written doesn't take into account, that when you've only set throwOnWarning, esbuild still expects a non-empty array (meaning it needs at least on item in it) in the returned object in the errors prop, so it wouldn't work as expected.

[KKoukiou]

So I rethought of this whole thing - and I believe we should not return the output of eslint into the errors/warnings back to esbuild.. Right now the errors are duplicated, from the console output and the esbuild failure. That's unnecessary and rather annoying especially if there many files contaning errors. It's enough to console logs the output, then the errors/warnings prop can contain only a dummy message.

Here is an example of the current output (see how much space the output duplication takes)

[kkoukiou@sequioa cockpit-podman]$ npm run build 

> build
> node build.js

/home/kkoukiou/repos/cockpit-podman/src/Containers.jsx
  3:1  error  More than 1 blank line not allowed  no-multiple-empty-lines

/home/kkoukiou/repos/cockpit-podman/src/DynamicListForm.jsx
  3:1  error  More than 1 blank line not allowed  no-multiple-empty-lines

✖ 2 problems (2 errors, 0 warnings)
  2 errors and 0 warnings potentially fixable with the `--fix` option.

✘ [ERROR] More than 1 blank line not allowed. [plugin eslint]

    /home/kkoukiou/repos/cockpit-podman/src/Containers.jsx:3:0:
      3 │ 
        ╵ ^

✘ [ERROR] More than 1 blank line not allowed. [plugin eslint]

    /home/kkoukiou/repos/cockpit-podman/src/DynamicListForm.jsx:3:0:
      3 │ 
        ╵ ^

2 errors
/home/kkoukiou/repos/cockpit-podman/node_modules/esbuild/lib/main.js:1636
  let error = new Error(`${text}${summary}`);
              ^

Error: Build failed with 2 errors:
/home/kkoukiou/repos/cockpit-podman/src/Containers.jsx:3:0: ERROR: [plugin: eslint] More than 1 blank line not allowed.
/home/kkoukiou/repos/cockpit-podman/src/DynamicListForm.jsx:3:0: ERROR: [plugin: eslint] More than 1 blank line not allowed.
    at failureErrorWithLog (/home/kkoukiou/repos/cockpit-podman/node_modules/esbuild/lib/main.js:1636:15)
    at /home/kkoukiou/repos/cockpit-podman/node_modules/esbuild/lib/main.js:1048:25
    at /home/kkoukiou/repos/cockpit-podman/node_modules/esbuild/lib/main.js:1512:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  errors: [
    {
      id: '',
      pluginName: 'eslint',
      text: 'More than 1 blank line not allowed.',
      location: {
        file: '/home/kkoukiou/repos/cockpit-podman/src/Containers.jsx',
        namespace: '',
        line: 3,
        column: 0,
        length: 0,
        lineText: '',
        suggestion: ''
      },
      notes: [],
      detail: -1
    },
    {
      id: '',
      pluginName: 'eslint',
      text: 'More than 1 blank line not allowed.',
      location: {
        file: '/home/kkoukiou/repos/cockpit-podman/src/DynamicListForm.jsx',
        namespace: '',
        line: 3,
        column: 0,
        length: 0,
        lineText: '',
        suggestion: ''
      },
      notes: [],
      detail: -1
    }
  ],
  warnings: []
}

So I propose, to never return the eslint output to esbuild - rather only print it in the console. Then the return value should looks like:

      return {
        ...warnings.length > 0 && { [throwOnWarnings ? 'errors' : 'warnings']: "Eslint warnings were found" },
        ...errors.length > 0 && { 'errors': "Eslint errors were found" },
        }

This code snippet above ignores the throwOnError property - as I believe it's redundant - and I would like to try to convince you once more about that.

eslint itself failes with non-zero exit code on errors

[kkoukiou@sequioa cockpit-podman]$ ./node_modules/eslint/bin/eslint.js src/Containers.jsx 

/home/kkoukiou/repos/cockpit-podman/src/Containers.jsx
  3:1  error  More than 1 blank line not allowed  no-multiple-empty-lines

✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

[kkoukiou@sequioa cockpit-podman]$ echo $?
1

By providing the throwOnErrors property and default it to 'false' you override that default behavior, which I believe is not correct.

Additionally eslint offers another option called max-warnings.

  --max-warnings Int              Number of warnings to trigger nonzero exit code - default: -1

That's somehow doing what 'throwOnWarning' does if you set it to 0. So I suggest to also remove throwOnWarning and introduce a maxWarnings property - in order to reflect what eslint offers.

So with my new proposal which is 100% aligned to what eslint does the code should look:

      return {
        ...warnings.length > 0 && { [maxWarnings < warnings.length ? 'errors' : 'warnings']: "Eslint warnings were found" },
        ...errors.length > 0 && { 'errors': "Eslint errors were found" },
        }

[robinloeffel]

hey @KKoukiou! thanks for all your feedback! i agree with you in the sense that forwarding all messages (meaning errors and warnings) to esbuild clutters up the console. i'll change the behavior to report just a dummy message back to esbuild.

however, i have to disagree with the rest of your points. if i were to go into the direction you're suggesting, i'd mix up the cli api and the node api. they're two different pairs of shoes. the node api, which is the one this, and every other eslint integration is using, doesn't have a maxErrors option, only the cli does, for example. eslint can't be configured in that way. here's an overview of the node options.

if you go further down the path of cli vs node api, you will notice that the node implementation doesn't fail when it encounters errors in the linted source code. if it did, it would throw, and i would have to use a try/catch block. only the cli does, as you've correctly stated.

both of the throw flags are strictly for the plugin itself, eslint doesn't do anything with them. the reason i've decided to add them, is feature parity between the esbuild plugin and other integrations. if you check out the official rollup plugin, for example, you will see the api is almost identical. the official webpack plugin has a different api, but allows the same behavior via fail flags.

none of them have the throw or fail options enabled as per default. it would just be too obtrusive during everyday development. if you wish to enable it for ci/cd environments, you can, and most definitely should. but for a default, it's too aggressive, and not what people would expect.