nuxt-community / nuxt-property-decorator

Property decorators for Nuxt (base on vue-property-decorator)
https://github.com/kaorun343/vue-property-decorator
MIT License
400 stars 34 forks source link

The question about hooks #80

Open Kasheftin opened 4 years ago

Kasheftin commented 4 years ago

I've just started using nuxt-property-decorator (I'm new with vue-property-decorator as well) and I would like to ask how you remember all the hooks. It's totally unclear (on the first glance) why some class methods are hooks and the other are just methods. For example,

@Component
export default class ErrorLayout extends Vue {
  @Prop({ type: Object, default: null }) readonly error!: NuxtError

  test() {
    return 'test'
  }

  layout() {
    return 'empty'
  }
}

Layout is a hook and placed in the root scope while test is a method and placed inside methods. Isn't it more intuitive to consider all the class's methods as vue methods and have something like @AtRoot decorator if anything has to be placed on the root level?

scscgit commented 3 years ago

I'm in the same situation; not only does this seem non-intuitive, but it also feels very dangerous, because a library may add a new hook at any point in the future, and we won't even notice it. This includes situations of migrating an existing project. (Honestly, isn't it obvious that people may choose single-word methods like "activated" with extremely large probability?) At the very least I'd prefer having some linter assert that no hooks were accidentally used, but there's no such thing at the moment (making it a pain to strive for defensive programming; we don't even have linters to assert basics like if used props exist). Arguably even riskier situation is when business logic depends on hooks being activated, e.g. beforeDestroy, however we lack any guarantee that such a method wasn't removed/renamed/mistyped. The Developer Experience would be so much better if we could strongly type all the hooks, providing IDE support and documentation, similar to the result of class implementing an interface containing the hook.

Also one more thing, I found that https://class-component.vuejs.org/guide/hooks-auto-complete.html documents import 'vue-class-component/hooks' as an auto-complete solution, so I'd like to ask if this isn't something that could be imported by this Nuxt library directly on the background (with the idea of better zero-configuration defaults). (Installing it as a Nuxt plugin that imports the dependency didn't seem to produce the expected result in IDE though.) And as an off-topic, the same could be asked about reflect-metadata, so I'm very curious to hear more on this topic.

husayt commented 3 years ago

This is a very valid question. Any suggestion or better merge request will be welcome.

As for Hooks autocomplete, I agree @scscgit and even was planning to add type safe definitions for all the nuxt specific hooks along with ones defined by vue-class-component. Something like this:

declare module 'vue/types/vue' {
  // Augment component instance type
  interface Vue {
    beforeRouteEnter?(
      to: Route,
      from: Route,
      next: (to?: RawLocation | false | ((vm: Vue) => void)) => void
    ): void

    beforeRouteLeave?(
      to: Route,
      from: Route,
      next: (to?: RawLocation | false | ((vm: Vue) => void)) => void
    ): void

    beforeRouteUpdate?(
      to: Route,
      from: Route,
      next: (to?: RawLocation | false | ((vm: Vue) => void)) => void
    ): void
  }
}
....

What slowed me down was the following: To get them autoregistered, I will probably need to turn this into a module. So far, we avoided this and just importing nuxt-property-decorator on use was enough. If we still can do this somehow, without turning library into a module, please, let me know. That will be preferred.

wiktor-obrebski commented 3 years ago

This includes situations of migrating an existing project. (Honestly, isn't it obvious that people may choose single-word methods like "activated" with extremely large probability?)

I just migrated middlesize project from vue-property-decorator to nuxt-property-decorator and I hit exactly this issue. It take me few hours to decode all problems scattered around the code related to name conflicts. Errors was quite confusing, different for various names.

husayt commented 3 years ago

@wiktor-obrebski can you mention problems you faced. We can solve them if we know and understand what is going on.

wiktor-obrebski commented 3 years ago

The problem I had was exactly like defined in @scscgit message. I had a project based on vue-property-generator. I migrated it to nuxt-property-generator. It gave me few strange errors that was hard to debug. But at the end I found that the problem source was name conflicts - e.g. I had a loading getter in a component. I had meta getter in second one. And more similar.

I resolved all the issues and do not looks for help. I left my comment just to confirm the case described in the issue.

husayt commented 3 years ago

Thanks @wiktor-obrebski, indeed we need to sort this out. If anyone has any suggestions, please.

wiktor-obrebski commented 3 years ago

maybe all hooks function should be implemented on base Vue class, with empty body and proper typing? by this it should be much less possible to declare some random properties / functions with same names. and in case of migration - TypeScript would warn the user that the properties are in conflict.

scscgit commented 3 years ago

I'll just note that personally I'd expect (and prefer) the hooks to be configurable via decorators, for example @Hook() created(), which would help avoid using one accidentally (maybe as a static analysis over @Component), and it could throw an error if no such hook exists. It'd be even better if we could even separate the namespace of methods and support loading() that isn't a hook, which would be a breaking change.

I could imagine this safety being an opt-in feature if we wanted to avoid breaking existing projects (and I'm hoping that a default setup with all such best practices will once be back-integrated into create-nuxt-app).