Skip to content

Inheriting from an abstract class causes decorators of subclasses to mix #444

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

Closed
davidbludlow opened this issue Sep 11, 2020 · 4 comments
Closed

Comments

@davidbludlow
Copy link

If abstract class AbstractBase extends Vue and class A extends AbstractBase and class B also extends AbstractBase, then class B will receive the decorators from class A, which is super bad and confusing.

The reason why this is an issue for abstract super classes (as opposed to not-abstract) is because TypeScript doesn't let you put a @Component decorator on abstract classes. So, whether the super class is abstract or not doesn't really matter, it just matters that there will not be a @Component decorator on that super class, which situation will cause this bug.

See the following test case

it('even if super class is abstract, should not get super and sub class decorators mixed up', async function() {
  // Watch function/decorator copied from
  // https://github.com/kaorun343/vue-property-decorator/blob/master/src/vue-property-decorator.ts
  function Watch(path: string, options: WatchOptions = {}) {
    const { deep = false, immediate = false } = options;

    return createDecorator((componentOptions, handler) => {
      if (typeof componentOptions.watch !== 'object') {
        componentOptions.watch = Object.create(null);
      }

      const watch: any = componentOptions.watch;

      if (typeof watch[path] === 'object' && !Array.isArray(watch[path])) {
        watch[path] = [watch[path]];
      } else if (typeof watch[path] === 'undefined') {
        watch[path] = [];
      }

      watch[path].push({ handler, deep, immediate });
    });
  }

  const spyBase = td.function('BaseNotify');
  const spyA = td.function('ANotify');
  const spyB = td.function('BNotify');
  const spyC = td.function('CNotify');

  // Since this is abstract, TS will not let us add the @Component decorator,
  // like we would normally do
  abstract class Base extends Vue {
    count = 0;

    @Watch('count')
    notify() {
      spyBase();
    }
  }

  @Component({})
  class A extends Base {
    notify() {
      spyA();
    }
  }

  @Component({ components: {} })
  class B extends Base {
    // Oh no! The super class also has a @Watch decorator for "count"!
    @Watch('count')
    notify() {
      spyB();
    }
  }

  @Component({ components: {} })
  class C extends Base {
    notify() {
      spyC();
    }
  }

  const vmA = new A();
  const vmB = new B();
  const vmC = new C();
  td.verify(spyBase(), { times: 0 });
  td.verify(spyA(), { times: 0 });
  td.verify(spyB(), { times: 0 });
  td.verify(spyC(), { times: 0 });

  vmA.count++;
  await vmA.$nextTick();
  td.verify(spyBase(), { times: 0 });
  td.verify(spyA(), { times: 1 });
  td.verify(spyB(), { times: 0 });
  td.verify(spyC(), { times: 0 });

  vmB.count++;
  await vmB.$nextTick();
  td.verify(spyBase(), { times: 0 });
  td.verify(spyA(), { times: 1 });
  // Someday it would be nice if we could change the line
  // below so that it said `1` not `2`
  td.verify(spyB(), { times: 2 });
  td.verify(spyC(), { times: 0 });

  vmC.count++;
  await vmA.$nextTick();
  td.verify(spyBase(), { times: 0 });
  td.verify(spyA(), { times: 1 });
  td.verify(spyB(), { times: 2 });
  td.verify(spyC(), { times: 1 });
});

That test will fail on the second to last line (td.verify(spyC(), { times: 1 });) with the error:

     Error: Unsatisfied verification on test double `CNotify`.

  Wanted:
    - called with `()` 1 time.

  All calls of the test double, in order were:
    - called with `()`.
    - called with `()`.
      at Object.fail (webpack:///./node_modules/testdouble/lib/log.js?:16:15)
      at Object.eval [as verify] (webpack:///./node_modules/testdouble/lib/verify.js?:20:23)
      at Context.eval (webpack:///./test/test.ts?:358:58)
@davidbludlow
Copy link
Author

A fix for this at #445 is awaiting review.

@davidbludlow
Copy link
Author

davidbludlow commented Sep 11, 2020

Some trivial, historical FYI:

A problem similar to this was fixed by Pull Request #200 , but that fix doesn't work in this case, because the new code in that pull request will not even execute for the super class, because the super class doesn't have any @Component decorator in the case this issue is about.

@ktsn
Copy link
Member

ktsn commented Sep 11, 2020

Every superclass must be decorated by @Component in <= v7. It's stated in the docs.
https://class-component.vuejs.org/guide/extend-and-mixins.html

It is already solved in version 8.

@ktsn ktsn closed this as completed Sep 11, 2020
@davidbludlow
Copy link
Author

How sad. I guess I can't use abstract superclasses right now, then. Thanks for the quick response, anyway!

Anyways, how is it looking for Version 8 and Vue 3? Will it be more-popular/best-practice to use Vue 3 with vue-class-component, as opposed to using Vue 3 without use vue-class-component ? Will most Vue 3 examples, documentation, and lessons be shown using vue-class-component (via using the kaorun343/vue-property-decorator package)?

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