QED0711 / spiccato

MIT License
0 stars 1 forks source link

For better typescript definitions, addCustomSetters() method might be the problem? #25

Open navarrotech opened 3 months ago

navarrotech commented 3 months ago

I was playing around with the types, and I found that if I set the custom setters in the constructor instead of the method, then the typing utilities worked perfectly!

Here's an example of the issue I was facing. Take this code and put it into a typescript

import Spiccato from '../src/index'

type Track = {
  id: number
  position: number[]
  name: string
}

type Store = {
  trackKeys: Set<number>
  tracksById: Record<string, Track>
}

const store: Store = {
  trackKeys: new Set([]),
  tracksById: {}
}

export const trackManager = new Spiccato(store, {
  id: 'tracks',
  enableWriteProtection: false
})

trackManager.init()
trackManager.addCustomSetters({
  setTrack(track: Track) {
    this.setState((prevState: Store) => {
      prevState.trackKeys.add(track.id)
      prevState.tracksById[track.id] = track

      return prevState
    })
  }
})

// HERE! Typescript throws an error, saying that `Property 'setTrack' does not exist on type {}`
trackManager.setters.setTrack({
  id: 1,
  position: [0, 0],
  name: 'Track 1'
})

This got me thinking, if Redux can do it so can Spiccato. And it should be able to infer all types from all methods defined in the custom setters!

I put together a simpler set of class code, as a test to see if I can get the typescript bindings to work. I found that if I have them set from the constructor directly, then typescript can infer those types.

type GetterMethod = (...args: any[]) => any;

class Testy<CustomGetters extends Record<string, GetterMethod> = {}> {
  public getters: CustomGetters;

  constructor(getters: CustomGetters) {
    this.getters = getters;
  }
}

const testy = new Testy({
  addCount: (addFirst: number, addSecond: number) => {
    return addFirst + addSecond;
  },
  subtractCount: (subFirst: number, subSecond: number) => {
    return subFirst - subSecond;
  }
});

// Now TypeScript knows the available methods on getters
testy.getters.addCount(1, 2); // This works
testy.getters.subtractCount(5, 3); // This works

This is making me conclude that Typescript cannot add more types after the class has already been defined? I think the core issue here is that more types are being added after the class has been created, and typescript will only show types that exist upon creation.

Now on the other hand, we could also have a type for all CustomGetters as I believe you've already done, however that's a lot of custom type defintions that could be inferred automatically upon creation.

My recommendation: Remove the "addCustomSetters" method and "addCustomGetters" methods. Shouldn't all custom setters and getters be defined when the Spiccato manager is defined? I'm struggling to think of a scenario where Spiccato custom setters would be set after init().

I also think that all dynamically generated setters/getters could also be generated in the constructor, which would then also enable all of those typescript types to be automatically generated too.

As a bonus of removing these, it would remove the possibility of calling these methods before "init"

QED0711 commented 3 months ago

Yes, I think the ideal solution would be to have TS infer the types of all these. This was something I tried to get going in several different ways, but I always came back to one issue. This only works if you supply the getters/setters/methods in the constructor as you've said. But specifically, it will only work if you apply those within the same scope as the initialize call on the spiccato instance.

Take this as an example - imagine a slightly different class definition whey you supply your getters/setters/methods as arguments:

manager = new Spiccato({
    customGetter(){
        this.methods.someCustomMethod() // 'this' can be inferred in this context 
    }.
   // other setters and methods here
})

That all works. But one of the fundamental ideas that I had when designing this (and one of the ways I use this on a daily basis) is to be able to use the file use the file system to help logically separate getter, setter, and method functionality. The funcionality I display in the docs is very simple, but in practice, these custom functions can be very complex, and run for dozens to hundreds of lines. If you have to define them all in the same file, that becomes unmanageable at scale.

So imagine you have some custom function you define in another file. If you try to access the this keyword in that function, you would have to explicitly define what this refers to get type safety. So you need a type that defines the signature of this. And if you have that, then you technically need to have types for State, Getters, Setters, Methods, and NamepsacedMethods in that context so you could access this.setters, for example.

This organization pattern is covered in the 1.0.2-beta branch. Use the CLI to generate the file structure for the state resource, and you'll see the whole scope of that pattern. It is more verbose, but does get the functionality to work in a way that allows for defining these things in an organized way between several files.

your comment aboutinitializing with custom setters/getters before init is right on. There is a reason that it's done this way right now (if you add them before init, it will not allow you to override dynamic setters/getters with custom functionality). But I think you're right. That should be changed to allow for init to be called after.

QED0711 commented 3 months ago

the issues with calling init() after addCustomGetters and addCustomSetters has been resolved in the release-1.0.4-beta branch. Will close this issue with a PR when we merge the beta line of branches into main.