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

Using this.error outside the transformation context #4424

Open
tzachbon opened this issue Mar 3, 2022 · 12 comments
Open

Using this.error outside the transformation context #4424

tzachbon opened this issue Mar 3, 2022 · 12 comments

Comments

@tzachbon
Copy link

tzachbon commented Mar 3, 2022

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?

@lukastaegert
Copy link
Member

lukastaegert commented Mar 3, 2022

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
Copy link
Member

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

@tzachbon
Copy link
Author

tzachbon commented Mar 3, 2022

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?

@lukastaegert
Copy link
Member

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
Copy link
Member

lukastaegert commented Mar 3, 2022

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
Copy link
Member

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

@tzachbon
Copy link
Author

tzachbon commented Mar 4, 2022

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.

@lukastaegert
Copy link
Member

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.

@tzachbon
Copy link
Author

tzachbon commented Mar 4, 2022

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.

@lukastaegert
Copy link
Member

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

@lukastaegert
Copy link
Member

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

@tzachbon
Copy link
Author

tzachbon commented Mar 6, 2022

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!

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

No branches or pull requests

2 participants