typescript: add base model typing

[hamiltoes]

Summary

Add explicit typing to BaseModel to aide development.

• Tell us about the problem your pull request is solving.
  Currently, the BaseModel has type any which makes intellisense nearly useless. I am adding interfaces with jsdoc strings to further improve the already great experience of developing with FeathersVuex.
• Are there any open issues that are related to this?
  no
• Is this PR dependent on PRs in other repos?
  no

Other Information

Because the BaseModel class is constructed inside a function, it's type cannot be used outside of that function. My solution is to define multiple interfaces: one for the static side ModelStatic, and one for the instance side ModelInstance. Both of these interfaces are generic and take a type parameter that describes the service's data structure. There is also a ModelInstanceClone interface for clones.

Ideally, we would just make the BaseModel returned from the FeathersVuex setup function generic, but I could not find a way to cast the non-generic BaseModel of type any to a generic interface. So my solution is to return BaseModel casted to a ModelStatic<{}> and an additional method castBaseModel from the setup function which can be used to add typing for each service's data interface.

Notes

• Model<D> is of type ModelInstance<D> & Readonly<D>
  • This enforces clone & commit pattern because the model data is readonly so TS will error when trying to directly alter model data.
• ModelClone<D> is of type ModelInstanceClone<D> & D
  • I hide commit() and reset() methods in ModelInstanceClone since they just error on non-clones.
  • Clone model data is not readonly so you can directly assign to it

Examples

If you want to check out how this works with intellisense, take a look at the following. Or even better, clone this branch, paste it in index.ts and play with it :-)

const { BaseModel, castBaseModel } = feathersVuex({}, { serverAlias: 'api' })

// Old way of doing things
class OldFashionedNote extends BaseModel {
  static readonly servicePath: 'old-fashioned-note'
}
const ofn = new OldFashionedNote()

// New way with types
interface NoteRecord {
  id: number
  text: string
  userId: number
  color: string
  isItBetter?: true
}

class FancyTypedNote extends castBaseModel<NoteRecord>() {
  static readonly servicePath = 'fancy-typed-note'

  static setupInstance(data: Partial<NoteRecord>, ctx: SetupContext) {
    const typedStoreAccess = ctx.store['fancy-typed-note'].isFindPending
    return data
  }
}
const ftn = new FancyTypedNote()
ftn.text = 'TS shows error because data is readonly!'

const ftnClone = ftn.clone()
ftnClone.text = 'Direct assignment to clones is fine!'

Screenshots

Autocomplete for OldFashionedNote instance

• No commit() or reset()
• also no typing for actual model
  

Autocomplete for OldFashionedNote class

• includes EventEmitter methods for Model events
  

Autocomplete for FancyTypedNote instance

• includes NoteRecord fields!
  

Augmentable interfaces

Users can also augment interfaces defined for feathers-vuex to add more helpful typing

interface MyApiModels {
  FancyTypedNote: typeof FancyTypedNote
  OldFashionedNote: typeof OldFashionedNote
}

// This example is pasted in `index.ts` but a user would actually use
// declare module 'feathers-vuex' {...
declare module '.' {
  interface FeathersVuexStoreState {
    'old-fashioned-note': ServiceState
    'fancy-typed-note': ServiceState
  }
  interface FeathersVuexGlobalModels {
    api: MyApiModels
  }
}

And then they can get autocomplete on Model.models, Model.store, Vue.$FeathersVuex, setupInstance's store and models params, etc.

[hamiltoes]

I have intentionally left out some methods and fields from the BaseModel class but if I am mistaken and those should be publicly visible, please let me know! (Or if other should be hidden)

• hydrateAll() - I've never used SSR but wasn't sure if this was called internally or by clients
• keepCopiesInStore - this is going to be deprecated
• merge() - under the impression this is internal use only

I'm probably forgetting a few...

[J3m5]

This is awesome! Thank you for your contribution.

It seems that there is a extra brace at the last line of the index.ts file and it makes the CI build fail.

[hamiltoes]

@J3m5 ah thank you! That's what I get for making my playground in index.ts 😛

[J3m5]

There are a lot of Typescript errors that came up in the CI build, can you take a look at it ?
Can they be caused by the changes in the tsconfig.json file ?

[hamiltoes]

@J3m5 yeah they definitely are. I think stricter null checking is good but enabling that is outside the scope of this PR so I'll revert that.

[hamiltoes]

So the next issue I want some opinions on is whether or not Model data should be readonly. This will a decent amount of test updates which I'm willing to do, but don't want to do if we decide to not make it readonly :-)

I hope the example below clarifies what I'm asking.

interface NoteRecord {
  id: number
  text: string
  // other props
}
class Note extends castBaseModel<NoteRecord>() {
  static modelName = 'Note'
}

const n = new Note()
n.text = 'this throws an error' // <-- TS error: NoteRecord props are readonly

const nClone = n.clone()
nClone.text = 'this is fine!' // NoteRecord props are not readonly in clone

Pros of making it readonly:

• enforces clone and commit pattern during development
• enforces Vuex strict mode compliance

Cons:

• will be a breaking change for everyone not using clone & commit
• adhering to vuex strict mode remains in the user's cognitive load instead of TS reminding them "hey you shouldn't directly edit this without cloning first"

Thoughts?

[J3m5]

Will this only impact typescript users ?
And could this be manually changed by overriding the types ?
( I'm not very familiar with Typescript. )

[hamiltoes]

Correct, only TS users will be affected by this. You can always cast away the readonly if you want

const n = new Note()
n.text = 'this throws an error' // <-- TS error: NoteRecord props are readonly

const casted = n as ModelClone<NoteRecord>
casted.text = 'this is fine' // no error

I will have to think about whether we could make this something the user can augment so they could globally disable it if they don't want it and don't want to cast everywhere.

[J3m5]

If this can be globally disabled, I think we can enforce it by default and add some info in the docs.
And maybe add those info as JSDocs directly, no ?

[hamiltoes]

There is now an exported interface FeathersVuexTypeOptions which users can augment. Specifically, users can now do the following to disable readonly model data

declare module 'feathers-vuex' {
  interface FeathersVuexTypeOptions {
    'model-readonly': false
  }
}

if model-readonly is anything other than false, model data will be readonly. Furthermore, if the user doesn't augment FeathersVuexTypeOptions, model data will be readonly.

[J3m5]

It looks good !

It seems that some tests like this one needs to be changed in order to follow the readonly constraint of the modelName property on the BaseModel.

And here the FeathersVuexGlobalModels need to be augmented, like mentionned here, if I understand correctly.

And I've noticed that the globalModels object is casted as FeathersVuexGlobalModels in some place but not in others, is this intended ?

[hamiltoes]

This is ready to merge pending feedback on readonly static props. Any thoughts @marshallswain, @J3m5 ?

[J3m5]

I think we should let them as they are, at least for now. What do you think @marshallswain ?

[marshallswain]

If static props are still mutable, then I'll go ahead and merge this.

[hamiltoes]

@marshallswain I've made all static props mutable except for store, models, and copiesById.

[hamiltoes]

This would address #519

[hamiltoes]

Brought up to date with latest master. I believe this is ready to merge if there is still interest.

[marshallswain]

Merged! Thanks @hamiltoes! This is a huge update. Thanks for your input, as always, @J3m5!