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

Replace vue-property-decorator with nuxt-property-decorator #31

Closed
AndrewBogdanovTSS opened this issue Feb 7, 2018 · 11 comments
Closed

Comments

@AndrewBogdanovTSS
Copy link

AndrewBogdanovTSS commented Feb 7, 2018

It's more of a suggestion rather than an issue. Since nuxt-property-decorator exists as a separate module that extends the nuxt-class component, maybe it would be better to use it inside template instead of vue-property-decorator ?

This question is available on Nuxt.js community (#c23)
@qm3ster
Copy link
Member

qm3ster commented Feb 7, 2018

I still have no idea what nuxt-property-decorator and nuxt-class-component bring to the table over vue-property-decorator and vue-class-component. Do you know?

@AndrewBogdanovTSS
Copy link
Author

Yes, let me explain. Those modules bring type checking for properties unique to Nuxt such as fetch or middleware functions on the Component object. Let's look at this code:

import Component from "vue-class-component"

@Component({
      fetch(){
        console.log("Fetch")
      }
    })

So if you'll import Component from vue-class-component you will get a TS error:

TS2345: Argument of type '{ fetch(): void; }' is not assignable to parameter of type 'VueClass<Vue>'.
  Object literal may only specify known properties, and 'fetch' does not exist in type 'VueClass<Vue>'.

Importing from nuxt-class-component you won't get such error

The same logic goes around nuxt-property-decorator. Though I've found a silly mistake in this module, it doesn't use nuxt-class-component internally, which should be corrected. I logged a related issue in their repo: nuxt-community/nuxt-property-decorator#1
Once that will be fixed we will be able to remove these unneeded dependencies:

vue-class-component
nuxt-class-component
vue-property-decorator

all of those are covered by
nuxt-property-decorator

Import of the component should be changed to
import { Component } from "nuxt-property-decorator"

@qm3ster
Copy link
Member

qm3ster commented Feb 10, 2018

Totally agree with import { Component } from "nuxt-property-decorator", that's how it was done with vue-property-decorator all along.

Not so sure about the whole nuxt- modules though:
Only "page" level components get these new hooks, so normal components should be vue-class-component

@AndrewBogdanovTSS
Copy link
Author

Yeah, but then again nuxt-class-component has vue-class-component as a dependency, so there is no reason to include it as a root level dependency

@qm3ster
Copy link
Member

qm3ster commented Feb 10, 2018

There's a point of view that anything you import directly in your code should be a dependency.
In some cases it makes sense to break this rule, for example with nuxt's vue.
I am not sure yet if this is one of those cases.

@AndrewBogdanovTSS
Copy link
Author

I'm not sure I've ever heard of such rule. What is the benefit of it?

@qm3ster
Copy link
Member

qm3ster commented Feb 10, 2018

  1. It doesn't matter what your dependencies import. If nuxt-class-component decides to rewrite for zero dependencies, vue-class-component won't disappear and break your code.
  2. Your direct dependency stays of the version range you specified, even if the same dependency changes version as a sub-dependency.

@AndrewBogdanovTSS
Copy link
Author

Yeah, makes sense

@AndrewBogdanovTSS
Copy link
Author

@qm3ster I've looked closely into an existing nuxt-property-decorator project's code and it doesn't make sense to me. As you've mentioned before it doesn't have any differences from vue-property-decorator. I logged an issue in it's repo about this but seems that an author of the module is busy with some other stuff and won't be able to support the lib properly. Maybe we should create a completely new module? I don't think it will be a complicated one. We can just import vue-property-decorator as a dependency and export Component from 'nuxt-class-component'. The gist of the module will look smth. like this:

// nuxt-property-decorator.ts
import Component from 'nuxt-class-component'
import {Constructor, Emit, Inject, Model, Prop, Provide, Watch} from 'vue-property-decorator'

export {Component, Constructor, Emit, Inject, Model, Prop, Provide, Watch}

what do you think about this?

@qm3ster
Copy link
Member

qm3ster commented Feb 12, 2018

Do you want to participate in maintaining that?
I fear that it's halfway between a "hook" into projects to add functionality in an update and indulging the "Not Invented Here" syndrome, similar to nuxt-link.

@AndrewBogdanovTSS
Copy link
Author

Yes, I agree that right now nuxt-property-decorator might be a bit accessive to have its own module, but logically it's a good option to have. Once Nuxt will evolve and will have more functionality and structure that module will become more and more to the point.

Do you want to participate in maintaining that?

Yes, sure. But I guess I will first try to push my suggestions to the existing repo to not create duplicated npm packages

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

3 participants