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

fix(errorHandler): async error handling for watchers #9484

Merged
merged 6 commits into from
Apr 16, 2021

Conversation

hello-brsd
Copy link
Contributor

@hello-brsd hello-brsd commented Feb 12, 2019

Added async error handling for watchers and immediate watchers

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Fixes #9625
Fixes #10009

@jfgodoy
Copy link

jfgodoy commented Mar 2, 2019

I'm waiting for this.
@hello-brsd you should squash your commits into one

@posva
Copy link
Member

posva commented Mar 2, 2019

no need to squash your commits, we already do when merging 😉

@hello-brsd hello-brsd changed the title feat(errorHandler): async error handling for watchers feat(errorHandler): async error handling for watchers (fix #9625) Mar 5, 2019
@hello-brsd hello-brsd changed the title feat(errorHandler): async error handling for watchers (fix #9625) feat(errorHandler): async error handling for watchers (fix #9625 #10009) May 23, 2019
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, sorry for the delay and thank you for this!
Could you add a test case in the errorCaptured.spec.js file as well? The test should pass already as the invokeWithErrorHandling already handles it, but it will be nice to have a test case for it 😀
The rest seems to be good!

@hello-brsd
Copy link
Contributor Author

@posva Thank you for the review, i added some extra test case to the errorCaptured.spec.js.

@abhipanda
Copy link

@posva - Is there an ETA for this merge ?

@mvirkkunen
Copy link

I just spent half an hour trying to figure out why my error handler wasn't getting called because I assumed all callback handling was already updated to support async functions. Would be really nice if this could be merged.

@Ishadimu
Copy link

Great work @hello-brsd! Until this is merged, I'm having to write an interim solution using axios interceptors along with vuex. 😬

@posva Thanks for approving the PR. Any updates on merge ETA?

@posva
Copy link
Member

posva commented Oct 14, 2019

Sorry, I cannot provide any ETA 😓

@caiquecastro
Copy link

Any updates?

@mvirkkunen
Copy link

I would also like to know what the status is on this because I've been bitten by this multiple times by now, and the fix seems very straightforward. I see Vue 2.x hasn't had a release with anything besides bug fixes for a while, so is this something you would even consider merging anymore? Or is all the focus on Vue 3 now?

@rogierpennick
Copy link

Any updates on this? Our error handling would be greatly improved by this because we use a lot of async watches in our code. I'd really like to avoid having to resort to workarounds!

@OzCroot
Copy link

OzCroot commented Jul 6, 2020

Can this please be reviewed?

@msokk
Copy link

msokk commented Sep 14, 2020

@OzCroot Looks like this is already approved a while ago, @posva any newer ETA for this to land?

Documentation for errorHandler says:

Assign a handler for uncaught errors during component render function and watchers.

and there is a 2.6.0+ update section that mentions added async support:

In addition, if any of the covered hooks or handlers returns a Promise chain (e.g. async functions), the error from that Promise chain will also be handled.

Overall one would expect from documentation that async watchers would work with global error handler, but currently it is not so 🙂

@berniegp
Copy link

Here`s a fix I came up with until this PR is merged.

// Fix "Vue.config.errorHandler and errorCaptured should capture rejected promised in watchers"
// https://github.com/vuejs/vue/issues/10009
// Ugly monkey patching until https://github.com/vuejs/vue/pull/9484 is merged
const realWatch = Vue.prototype.$watch;
Vue.prototype.$watch = function $watchDetour(expOrFn, callback, options) {
  if (typeof callback === 'function') {
    if (this.$_watchCount === undefined) {
      this.$_watchCount = 0;
    }

    // Reroute $watch to $on which handles promise rejections properly
    const watchDetourEvent = `watchDetour_${this.$_watchCount}`;
    this.$_watchCount += 1;
    const realCallback = callback;
    this.$on(watchDetourEvent, (args) => { return realCallback.apply(this, args); });
    callback = (...args) => { this.$emit(watchDetourEvent, args); };
  }

  // realWatch will call back into $watchDetour with a function if callback wasn't one
  return realWatch.call(this, expOrFn, callback, options);
};

@posva posva changed the title feat(errorHandler): async error handling for watchers (fix #9625 #10009) feat(errorHandler): async error handling for watchers Feb 24, 2021
@posva posva changed the title feat(errorHandler): async error handling for watchers fix(errorHandler): async error handling for watchers Apr 7, 2021
@posva posva removed the semver:minor label Apr 7, 2021
@posva posva merged commit e4dea59 into vuejs:dev Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet