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

feat(runtime-core): expose exception if app.errorHandler throws exception #987

Closed

Conversation

pikax
Copy link
Member

@pikax pikax commented Apr 17, 2020

Problem

If any of app.context.errorHandler or app.context.warnHandler throws an exception it will call it self over and over.

This behaviour is problematic specially when doing asserts in the setup, making the test to pass and the error to be obfuscated.

// not currently failing
// failing after this PR
  it("fail", () => {
    createVue({
      setup() {
        expect(1).toBe(2);
      },
    }).mount();
  });



export const createVue = (component: Component, props?: any) => {
  const app = createApp(component, props);

  const el = document.createElement("div");

  const mount = () => app.mount(el);

  const destroy = () => app.unmount(el);

  app.config.errorHandler = (err: any) => {
    throw err;
  };

  return {
    el,
    mount,
    destroy
  };
};

https://github.com/pikax/vue-composable/blob/vue3/packages/web/__tests__/utils.ts#L5-L23

jest log of the build, the file broadcastChannel.spec.ts should have tests failing, but they are passing and just logging error.

NOTE: [Vue warn]: Unhandled error during execution of app errorHandler at <Anonymous> (Root) is caused by the errorHandler exception

PASS  packages/web/__tests__/web/broadcastChannel.spec.ts
  ● Console

    console.warn node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:39
      [Vue warn]: Unhandled error during execution of app errorHandler 
        at <Anonymous> (Root)
    console.error node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:212
      JestAssertionError: expect(received).toBe(expected) // Object.is equality
      
      If it should pass with deep equality, replace "toBe" with "toStrictEqual"
      
      Expected: {"test": 1}
      Received: serializes to the same string
          at setup (/root/repo/packages/web/__tests__/web/broadcastChannel.spec.ts:109:28)
          at callWithErrorHandling (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:153:22)
          at setupStatefulComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:4818:29)
          at setupComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:4780:11)
          at mountComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2686:9)
          at processComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2665:17)
          at patch (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2382:21)
          at render (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:3379:13)
          at mount (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:1989:25)
          at Object.app.mount (/root/repo/node_modules/@vue/runtime-dom/dist/runtime-dom.cjs.js:1550:23)
          at Object.mount (/root/repo/packages/web/__tests__/utils.ts:10:27)
          at Object.<anonymous> (/root/repo/packages/web/__tests__/web/broadcastChannel.spec.ts:112:8)
          at Object.asyncJestTest (/root/repo/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
          at /root/repo/node_modules/jest-jasmine2/build/queueRunner.js:45:12
          at new Promise (<anonymous>)
          at mapper (/root/repo/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
          at /root/repo/node_modules/jest-jasmine2/build/queueRunner.js:75:41 {
        matcherResult: {
          actual: { test: 1 },
          expected: { test: 1 },
          message: [Function],
          name: 'toBe',
          pass: false
        }
      }
    console.warn node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:39
      [Vue warn]: Unhandled error during execution of app errorHandler 
        at <Anonymous> (Root)
    console.error node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:212
      JestAssertionError: expect(received).toBe(expected) // Object.is equality
      
      If it should pass with deep equality, replace "toBe" with "toStrictEqual"
      
      Expected: {"test": 1}
      Received: serializes to the same string
          at setup (/root/repo/packages/web/__tests__/web/broadcastChannel.spec.ts:130:34)
          at callWithErrorHandling (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:153:22)
          at setupStatefulComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:4818:29)
          at setupComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:4780:11)
          at mountComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2686:9)
          at processComponent (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2665:17)
          at patch (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:2382:21)
          at render (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:3379:13)
          at mount (/root/repo/node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:1989:25)
          at Object.app.mount (/root/repo/node_modules/@vue/runtime-dom/dist/runtime-dom.cjs.js:1550:23)
          at Object.mount (/root/repo/packages/web/__tests__/utils.ts:10:27)
          at Object.<anonymous> (/root/repo/packages/web/__tests__/web/broadcastChannel.spec.ts:136:8)
          at Object.asyncJestTest (/root/repo/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
          at /root/repo/node_modules/jest-jasmine2/build/queueRunner.js:45:12
          at new Promise (<anonymous>)
          at mapper (/root/repo/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
          at /root/repo/node_modules/jest-jasmine2/build/queueRunner.js:75:41 {
        matcherResult: {
          actual: { test: 1 },
          expected: { test: 1 },
          message: [Function],
          name: 'toBe',
          pass: false
        }
      }

@yyx990803
Copy link
Member

yyx990803 commented Apr 20, 2020

If you expect it to just throw, what's the point of adding app.config.errorHandler in this case then?

@yyx990803
Copy link
Member

I don't think we should expose the exception - this would cause any error in the error handler to crash the app, which defeats the purpose of having an error handler.

@yyx990803 yyx990803 closed this Apr 20, 2020
@pikax
Copy link
Member Author

pikax commented Apr 20, 2020

The reason is to expose the place where the exception happen, not exposing the exception will make the test pass but to log the exception.

import { createApp } from "vue";

describe("test", () => {
  it("test", () => {
    createApp({
      setup() {
        expect(1).toBe(2);
      },
    }).mount(document.createElement("div"));
  });
});

image

Rethrowing the exception will make the error to be in the correct place:

import { createApp } from "../utils";

describe("test", () => {
  it("test", () => {
    const app = createApp({
      setup() {
        expect(1).toBe(2);
      },
    });

    app.config.errorHandler = (e) => {
      throw e;
    };

    app.mount(document.createElement("div"));
  });
});

image

That's the behaviour I would expect

@yyx990803
Copy link
Member

My point is in your test setup you don't need the errorHandler at all - then it will just throw as expected.

@pikax
Copy link
Member Author

pikax commented Apr 20, 2020

My point is in your test setup you don't need the errorHandler at all - then it will just throw as expected.

It only throws during production, because I'm running my tests in __DEV__, is not throwing. Although runtime-core.cjs.js doesn't even have the branch to throw exception


The way I'm doing in v2, setup my tests for the composition API, without it I encounter the same issue as in v3

// jest.config.js
Vue.config.warnHandler = (err: any) => {
  throw err;
};

if I do something similar in v3 I get: Maximum call stack size exceeded

 FAIL  packages/vue-composable/__tests__/web/test.spec.ts
  test
    × test (19ms)

  ● test › test

    RangeError: Maximum call stack size exceeded

      at Object.pauseTracking (node_modules/@vue/reactivity/dist/reactivity.cjs.js:74:23)
      at warn (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:18:16)
      at logError (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:214:9)
      at handleError (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:205:5)
      at callWithErrorHandling (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:159:9)      
      at warn (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:23:9)
      at logError (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:214:9)
      at handleError (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:205:5)
      at callWithErrorHandling (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:159:9)      
      at warn (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:23:9)
      at logError (node_modules/@vue/runtime-core/dist/runtime-core.cjs.js:214:9)

Other way to put this PR is:
If you throw an exception inside of the errorHandler, it shouldn't call itself again with the exception.


I also check the logError method to see if there's any way to throw:

Checked the code and the logError looks to have the __TEST__ flag in there , but the compiled code looks like this:

//runtime-core.esm-bundler.js
function logError(err, type, contextVNode) {
    // default behavior is crash in prod & test, recover in dev.
    if ((process.env.NODE_ENV !== 'production') && ( !false)) {
   // warn log
    }
    else {
        throw err;
    }
}

// runtime-core.cjs.js
// default behavior is crash in prod & test, recover in dev.
function logError(err, type, contextVNode) {
    // default behavior is crash in prod & test, recover in dev.
    {
        const info = ErrorTypeStrings[type];
 // warn log
    }
}

Not allowing it to throw, the only way is to run tests in production and use esm, is that the way you recommend? Or is there any other way to do it?

@pikax pikax deleted the feat/re-throw-error-on_error_handler branch May 6, 2020 07:53
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

Successfully merging this pull request may close these issues.

2 participants