createLocalVue errorHandler Option #1670

AtofStryker · 2020-08-28T21:11:08Z

The purpose of this PR is to allow an errorHandler option for createLocalVue. The goal is to support an interface to allow for support over time as needed for Vue Global Config options.

createLocalVue({
  errorHandler(err, vm, info) {
    // catch errors on local vue instance here
  }
})

createLocalVue used to take a private argument, which was a vue instance. The public API does not use this _vue instance argument, but server-test-utils and test-utils both do. The first step to supporting this signature was to move create-local-vue to the shared directory and use the semantic _createLocalVue when referencing internally. The "new" createLocalVue (part of the public api) just wraps this by passing through undefined to get the default behavior as well as the propagated VueConfig.

It is worth noting that the VTU behavior for disallowing a global error handler is still valid. The way this feature works is by invoking the registered errorHandler in the VTU global error handler before throwing the error. The function traverses the components $parent up the tree, finds if a localVue instance exists, and if so, invoke the errorHandler option if one is defined. That can be seen here. The user defined errorHandler passed into localVue can also throw an error if desired, but is not recommended.

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

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:

It's submitted to the dev branch.
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
All tests are passing: https://github.com/vuejs/vue-test-utils/blob/dev/.github/CONTRIBUTING.md#development-setup
New/updated tests are included

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): Addresses Async exceptions in beforeMount not caught in unit tests #1503

Other information:

Rename createLocalVue to _createLocalVue to indicate private use

Allow VueConfig to be passed in via createLocalVue to be registered on the created vue instance

add findAllParentInstances method to traverse a component's parent to find globally registered properties via createLocalVue

Pass localVue into mounted createLocalVue to register localVue properties on component

Call the user defined errorHandler created on the localVue instance via createLocalVue. This is called in the VTU global error handler

add tests to createLocalVue errorHandler to test invocation on sync and async throws

Wrap async error test for createLocalVue errorHandler in try/finally to prevent global errors

Add additional type safety to find and error for older versions of vue that might propagate a vm value of null/undefined

AtofStryker · 2020-08-28T23:25:08Z

packages/test-utils/src/error.js

  const error =
    typeof errorOrString === 'object' ? errorOrString : new Error(errorOrString)

+  // If a user defined errorHandler was register via createLocalVue
+  // find and call the user defined errorHandler
+  const instancedErrorHandlers = findAllParentInstances(vm)


maybe just move this into line 21 that already has the conditional check? A lot of the type checking is for older versions of Vue

AtofStryker · 2020-08-28T23:25:24Z

packages/test-utils/src/find.js

+export function findAllParentInstances(childVm: any) {
+  const instances = [childVm]
+
+  function getParent(_vm) {


Any ideas on how to avoid the closure here?

This is to find the highest Vue with the custom errorHandler right? I somehow thought it would "just work" and we wouldn't need to reverse-traverse all the way up. I don't have another way to do this.

I think having the conditional is fine, no problem from my point of view

@lmiller1990 correct. I thought it would work straight out of the box as well, but not exactly for reasons I am having trouble remembering. I want to say it has to do with how the vue instances behave on the component, but don't take my word for it 😅

AtofStryker · 2020-08-28T23:39:40Z

packages/shared/create-local-vue.js

+  instance.config = cloneDeep(Vue.config)
+
+  // if a user defined errorHandler is defined by a localVue instance via createLocalVue, register it
+  instance.config.errorHandler = config.errorHandler || Vue.config.errorHandler


This is the only line that has changed within the logic, IE adding config.errorHandler. If we do not feel confortable reviewing this due to scope of change, we can break out the shared move in a separate PR to more clearly see the change set

shared is private anyway, this should be fine

AtofStryker · 2020-08-28T23:40:01Z

packages/shared/create-local-vue.js

+ */
+function _createLocalVue(
+  _Vue: Component = Vue,
+  config: VueConfig = {}


added argument config: VueConfig = {}

lmiller1990 · 2020-08-29T10:09:23Z

Hi! This is a pretty large PR, please give me the weekend to review. I am excited for this feature.

AtofStryker · 2020-08-30T00:10:31Z

Hi! This is a pretty large PR, please give me the weekend to review. I am excited for this feature.

@lmiller1990 sure thing! I am excited about it too. Definitely no rush on review as we want to be thorough.

What we can do, since this is a fairly large PR where the change set is a bit difficult to determine, is break this out into 2 separate PRs:

1st PR (break out createLocalVue to shared and wrap the functionality around the new createLocalVue)

48f1c14
808fe89
fc3be6c
0dca580

2nd (implement the errorHandler)
everything else

I think this would be much easier to review as well as add confidence to what we are doing. What do you think?

lmiller1990 · 2020-08-30T12:21:51Z

I am happy to review a big PR, no dramas there.

AtofStryker · 2020-08-30T14:58:46Z

@lmiller1990 OK cool! I just want to make it as easy as possible and willing to accommodate 😃

lmiller1990 · 2020-09-08T06:37:41Z

Sorry about the wait, I have a day off work this week to work on OSS (mainly VTU stuff).

I will review/merge and plan release 1.1 for this weekend.

AtofStryker · 2020-09-08T14:30:16Z

@lmiller1990 no worries! Let me know if there is anything I can do to help.

lmiller1990

Looks fine to me - this was a ton of work! Great job. I wonder if this will be easier to do in VTU next. I was not expecting this to be so complicated (I don't think you were, either).

lmiller1990 · 2020-09-10T01:41:00Z

packages/shared/create-local-vue.js

+  // e.g. router-link are created using the correct base constructor
+  instance.options._base = instance
+
+  // compat for vue-router < 2.7.1 where it does not allow multiple installs


nice job, this must have been a huge pain to figure out

oh, it was moved here from somewhere else - even better, that means it's been battle tested

lmiller1990 · 2020-09-10T01:41:52Z

packages/shared/create-local-vue.js

+  instance.config = cloneDeep(Vue.config)
+
+  // if a user defined errorHandler is defined by a localVue instance via createLocalVue, register it
+  instance.config.errorHandler = config.errorHandler || Vue.config.errorHandler


shared is private anyway, this should be fine

lmiller1990 · 2020-09-10T01:44:48Z

packages/test-utils/src/find.js

+export function findAllParentInstances(childVm: any) {
+  const instances = [childVm]
+
+  function getParent(_vm) {


This is to find the highest Vue with the custom errorHandler right? I somehow thought it would "just work" and we wouldn't need to reverse-traverse all the way up. I don't have another way to do this.

I think having the conditional is fine, no problem from my point of view

AtofStryker · 2020-09-10T03:43:01Z

Looks fine to me - this was a ton of work! Great job. I wonder if this will be easier to do in VTU next. I was not expecting this to be so complicated (I don't think you were, either).

@lmiller1990 Thanks! Yeah... this was a lot more involved than I thought it would be 😅 . The first approach I took failed miserably 😆 . This seemed to be the way to get the behavior we need with the least amount of change (even though significant change was needed). I will say this is WAYYYY easier with VTU Next. This is what it looks like in VTU Next, which works exactly like you would expect it to 🙂 .

lmiller1990 · 2020-09-10T04:49:35Z

Oh yeah. Forgot about that thread in VTU next.

Anyway, I released 1.1.0, thanks for your help! Pretty significant release.

thibaut-lo · 2020-10-01T13:29:06Z

Shouldn't export declare function createLocalVue (): typeof Vue be updated (https://github.com/vuejs/vue-test-utils/blob/dev/packages/test-utils/types/index.d.ts#L172) ?

coolboy0961 · 2021-07-06T13:14:41Z

Shouldn't export declare function createLocalVue (): typeof Vue be updated (https://github.com/vuejs/vue-test-utils/blob/dev/packages/test-utils/types/index.d.ts#L172) ?

Because this, I can not test errorHandler by Typescript.

lmiller1990 · 2021-07-07T04:50:39Z

I don't know what you mean, but if you'd like to improve the types, please make a PR. Alternatively, if you don't know how to fix them, you can open an issue sharing the problem you are having. Thanks!

coolboy0961 · 2021-07-11T05:01:46Z

@lmiller1990
Thank you for the response. I added a issue.
#1872

I think it just need to fix the type file, I wanted to make a PR,
but I do not have the permission to push the fix branch.

coolboy0961 · 2021-07-11T09:43:11Z

@lmiller1990
I made a PR
#1873

It turned out to be Fork first and then create a pull request.

AtofStryker added 17 commits August 25, 2020 22:29

refactor(createlocalvue): move createLocalVue to shared utils

48f1c14

refactor(createlocalvue): rename createLocalVue to _createLocalVue

808fe89

Rename createLocalVue to _createLocalVue to indicate private use

improvement(components): add Sync and Async components for testing

98103d6

improvement(flow): add VueConfig to Flow

fc3be6c

improvement(index): export new createLocalVue as default in Index

0dca580

improvement(_createlocalvue): allow registration of user defined config

094e7db

Allow VueConfig to be passed in via createLocalVue to be registered on the created vue instance

improvement(find): add findAllParentInstances to the find API

3db2482

add findAllParentInstances method to traverse a component's parent to find globally registered properties via createLocalVue

improvement(mount): pass localVue into mounted createLocalVue

f1005a9

Pass localVue into mounted createLocalVue to register localVue properties on component

improvement(error): call user defined errorHandler if defined

72f1fdb

Call the user defined errorHandler created on the localVue instance via createLocalVue. This is called in the VTU global error handler

improvement(createlocalvue): add tests to createLocalVue errorHandler

55647c4

add tests to createLocalVue errorHandler to test invocation on sync and async throws

docs(createlocalvue): document public createLocalVue API

8bbc3d1

docs(createlocalvue): document the createLocalVue internal API

75d0afc

docs(createlocalvue): document the errorHandler option in createLocalVue

ba9f677

fix(createlocalvue tests): wrap createLocalVue async test in try/finally

3272a85

Wrap async error test for createLocalVue errorHandler in try/finally to prevent global errors

fix(createlocalvue): skip async component throws for vue < 2.6

5783bb6

improvement(find and error): add additional type safety to find & error

5ed0f28

Add additional type safety to find and error for older versions of vue that might propagate a vm value of null/undefined

fix(createlocalvue): only run sync error tests for vue versions < 2.4

d73e8d9

AtofStryker commented Aug 28, 2020

View reviewed changes

AtofStryker marked this pull request as ready for review August 28, 2020 23:25

AtofStryker mentioned this pull request Aug 28, 2020

Async exceptions in beforeMount not caught in unit tests #1503

Closed

AtofStryker commented Aug 28, 2020

View reviewed changes

lmiller1990 self-requested a review August 29, 2020 10:09

lmiller1990 approved these changes Sep 10, 2020

View reviewed changes

lmiller1990 merged commit e91effe into vuejs:dev Sep 10, 2020

antoinerey mentioned this pull request Feb 5, 2021

Infinite loop on uncaught error when using createLocalVue() #1768

Closed

coolboy0961 mentioned this pull request Jul 11, 2021

Can not add createLocalVue errorHandler Option by typescript #1872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

createLocalVue errorHandler Option #1670

createLocalVue errorHandler Option #1670

AtofStryker commented Aug 28, 2020 •

edited

Loading

AtofStryker Aug 28, 2020

AtofStryker Aug 28, 2020

lmiller1990 Sep 10, 2020

AtofStryker Sep 10, 2020

AtofStryker Aug 28, 2020

lmiller1990 Sep 10, 2020

AtofStryker Aug 28, 2020

lmiller1990 commented Aug 29, 2020

AtofStryker commented Aug 30, 2020

lmiller1990 commented Aug 30, 2020

AtofStryker commented Aug 30, 2020

lmiller1990 commented Sep 8, 2020

AtofStryker commented Sep 8, 2020

lmiller1990 left a comment •

edited

Loading

lmiller1990 Sep 10, 2020

lmiller1990 Sep 10, 2020

lmiller1990 Sep 10, 2020

lmiller1990 Sep 10, 2020

AtofStryker commented Sep 10, 2020

lmiller1990 commented Sep 10, 2020

thibaut-lo commented Oct 1, 2020

coolboy0961 commented Jul 6, 2021

lmiller1990 commented Jul 7, 2021 •

edited

Loading

coolboy0961 commented Jul 11, 2021

coolboy0961 commented Jul 11, 2021

createLocalVue errorHandler Option #1670

createLocalVue errorHandler Option #1670

Conversation

AtofStryker commented Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Aug 29, 2020

AtofStryker commented Aug 30, 2020

lmiller1990 commented Aug 30, 2020

AtofStryker commented Aug 30, 2020

lmiller1990 commented Sep 8, 2020

AtofStryker commented Sep 8, 2020

lmiller1990 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented Sep 10, 2020

lmiller1990 commented Sep 10, 2020

thibaut-lo commented Oct 1, 2020

coolboy0961 commented Jul 6, 2021

lmiller1990 commented Jul 7, 2021 • edited Loading

coolboy0961 commented Jul 11, 2021

coolboy0961 commented Jul 11, 2021

AtofStryker commented Aug 28, 2020 •

edited

Loading

lmiller1990 left a comment •

edited

Loading

lmiller1990 commented Jul 7, 2021 •

edited

Loading