rollup icon indicating copy to clipboard operation
rollup copied to clipboard

Using `this.error` outside the transformation context

Open tzachbon opened this issue 3 years ago • 12 comments

We want to run some processes in buildStart and watchChange in a plugin that I work on. The process needs to emit diagnostics and wish to use the rollup API to show the user those diagnostics, but it always throws when we use this.error outside the transform hook.

Is there a way to use this API and recover from the error in other hooks?

tzachbon avatar Mar 03 '22 12:03 tzachbon

No. What this.error does is more or less just throw an Error, but add some additional attributes to the Error instance.

But you can use this.warn.

lukastaegert avatar Mar 03 '22 13:03 lukastaegert

Out of curiosity: Is it possible to use this.error in the transform hook without failing the build? That should actually not be possible.

lukastaegert avatar Mar 03 '22 13:03 lukastaegert

I guess there should be a distinction between failing the build and exiting the whole process with code 1. For example, if the user is using watch mode you don't want to kill the process also if there was an error (To give the user the chance to fix the problem). When you use this.error inside the transform hook it works as stated, but when used in watchChange it kills the process.

Is there a way to mimic this behavior inside other hooks such as watchChange and buildStart?

tzachbon avatar Mar 03 '22 14:03 tzachbon

It should not abort watchMode if you throw in buildStart, but maybe it does? But still, why not use this.warn? Or is the important part that you DO want to abort the build?

lukastaegert avatar Mar 03 '22 14:03 lukastaegert

Ok, in my experiments, throwing in buildStart does NOT abort watch mode and thus should behave exactly like transform or load etc. It is only throwing in watchChange that aborts watch mode. So is your issue actually that throwing in watchChange should not abort watch mode but just display the error?

lukastaegert avatar Mar 03 '22 14:03 lukastaegert

Created #4427 as a fix, please give it a go

lukastaegert avatar Mar 04 '22 09:03 lukastaegert

Ok, in my experiments, throwing in buildStart does NOT abort watch mode and thus should behave exactly like transform or load etc. It is only throwing in watchChange that aborts watch mode. So is your issue actually that throwing in watchChange should not abort watch mode but just display the error?

So regarding this, I did my investigation and you are right, the problem was that I was using this.error before I used this.addWatchFile, when I switch between them it worked in the buildStart. So thanks for that :)

Created #4427 as a fix, please give it a go

But as you stated, I still have a problem with the watchChange, I tried to fork this repo and use the branch from this PR and it did not work on my example (Link to the repo), unfortunately.

tzachbon avatar Mar 04 '22 10:03 tzachbon

I tried to fork this repo and use the branch

I was wondering why you did not simply run npm install rollup/rollup#gh-4424_throw_watch_change as the bot suggest in the PR, but it turns out this was broken for npm 7, I fixed this now on the branch.

Otherwise, it kind of works for me (in the least, the watch process was never aborted), except for two things:

function onBuildStartPlugin() {
  return {
    name: 'onWatchThrow',
    async buildStart() {
      this.addWatchFile(filePath);
      emitError();
    },
    async watchChange() {
      emitError();
    },
  };
}
function emitError() {
  const file = readFileSync(filePath, 'utf8');

  if (file.includes("console.log('error')")) {
    this.error(new Error('Stop the process'));
  }
}
  1. Just calling emitError() will have this set to the wrong value. If you replace it above with emitError.call(this), the correct errors are thrown.
  2. watchChange is a synchronous hook, therefore due to your use of async I got your errors as unhandled Promise rejections. The reason is that the watcher uses events to communicate, i.e. the watchChange hook will be triggered via an event, and we cannot simply wait for an event listener Promise without massively rearchitecting stuff.

lukastaegert avatar Mar 04 '22 13:03 lukastaegert

I tried to fork this repo and use the branch

I was wondering why you did not simply run npm install rollup/rollup#gh-4424_throw_watch_change as the bot suggest in the PR, but it turns out this was broken for npm 7, I fixed this now on the branch.

Otherwise, it kind of works for me (in the least, the watch process was never aborted), except for two things:


function onBuildStartPlugin() {

  return {

    name: 'onWatchThrow',

    async buildStart() {

      this.addWatchFile(filePath);

      emitError();

    },

    async watchChange() {

      emitError();

    },

  };

}

function emitError() {

  const file = readFileSync(filePath, 'utf8');



  if (file.includes("console.log('error')")) {

    this.error(new Error('Stop the process'));

  }

}

  1. Just calling emitError() will have this set to the wrong value. If you replace it above with emitError.call(this), the correct errors are thrown.

  2. watchChange is a synchronous hook, therefore due to your use of async I got your errors as unhandled Promise rejections. The reason is that the watcher uses events to communicate, i.e. the watchChange hook will be triggered via an event, and we cannot simply wait for an event listener Promise without massively rearchitecting stuff.

  1. My bad :), tried to refactor the code to be cleaner and push it as fast as possible and it got messed up, but I see you got the point regardless.

  2. I see, so this is pretty bad news for our case. We will still have to find a way to run the process which is asynchronous and still report the diagnostics for the user.

The preferred way is use the rollup api of course but I don't see how it's possible.

tzachbon avatar Mar 04 '22 19:03 tzachbon

I will see how much work it would be to allow the watchChange hook to become async.

lukastaegert avatar Mar 05 '22 12:03 lukastaegert

I pushed a potential solution to #4427 which will make the watchChange and closeWatcher hooks async.

lukastaegert avatar Mar 05 '22 13:03 lukastaegert

I pushed a potential solution to #4427 which will make the watchChange and closeWatcher hooks async.

Amazing, I tried this branch and it works as expected!

tzachbon avatar Mar 06 '22 09:03 tzachbon