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

Inconsistent logging for plugin errors (result.errors.push vs throw) #2625

Closed
ArnaudBarre opened this issue Oct 22, 2022 · 1 comment · Fixed by #2816
Closed

Inconsistent logging for plugin errors (result.errors.push vs throw) #2625

ArnaudBarre opened this issue Oct 22, 2022 · 1 comment · Fixed by #2816
Labels

Comments

@ArnaudBarre
Copy link

Repro:

build({
  entryPoints: ["./file.ts"],
  write: false,
  plugins: [
    {
      name: "test",
      setup(b) {
        b.onEnd((result) => {
          // throw new Error("Error description"); // <-- This get a nice log
          // This is not logged (but correctly fails the build)
          result.errors.push({
            pluginName: "test",
            id: "some-id",
            text: "Error description",
            detail: null,
            notes: [],
            location: null,
          });
        });
      },
    },
  ],
});
@evanw
Copy link
Owner

evanw commented Dec 3, 2022

This isn't really the right way to do this. At this point the build has already pretty much ended. The result object has already been generated. What you are suggesting would require esbuild to re-scan over the result object after every plugin and try to see which errors are things that should be logged. But it's not obvious or clear which ones should be. Just the ones added after the original length of the array (what if a plugin replaces an error instead of adding one)? Just the ones that are different (should this check shallow equality or deep)? This is also pointlessly inefficient.

The best way to fix this to me seems to be to allow onEnd to return errors and warnings like the other APIs:

// These all work:
b.onStart(() => ({ errors: [{ text: 'onStart' }] }))
b.onResolve({ filter: /./ }, () => ({ errors: [{ text: 'onResolve' }] }))
b.onLoad({ filter: /./ }, () => ({ errors: [{ text: 'onLoad' }] }))

// This currently doesn't work:
b.onEnd(() => ({ errors: [{ text: 'onEnd' }] }))

This is a breaking change because it would mean breaking the Go API, so it would have to wait for a breaking change release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants