QED0711 / spiccato

MIT License
0 stars 1 forks source link

Small typescript suggested changes as I read through the code #24

Closed navarrotech closed 3 months ago

navarrotech commented 3 months ago

Just a few things I noticed.

  1. Import optimization import can be set to import type for production optimizing

  2. Cleaner types

    
    interface Foo = {
    }
    // Same thing but as a TS utility type:
    type Foo = Record<string, number>

// And it works well with intersections: type Bazz = Record<string, any> & { noop: 'noop' | 'foo' }


3. Returning this
By ending some methods with `return this` it should allow for daisy-chaining of commands, so Spiccato can be written more consisely:
```typescript
const manager = new Spicatto();
manager
    .init()
    .addCustomSetters(/* methods */)
    .addCustomGetters(/* methods */)
    .addEventListener(/* methods */)
    .futureMethod(/*  */)

Just a few small things I noticed I could contribute to, while learning & reading the code.

Take it or leave it :)

QED0711 commented 3 months ago

love these suggestions! Especially the chaining. Will pull down to check on some test situations I haven't implemented yet and then will merge. Thanks!

QED0711 commented 3 months ago

Updated the tests to use the new chaining scheme you implemented. Ran into some typescript errors when I did that. The reason was this:

init(): Spiccato {
   // Code here
   return this
}

This tells typescript that a Spiccato instance is being returned with all the default generic values. So if you try to call something like this:

manager.init()
    .addCustomGetters({
        customGetter() {
            this.state.someVal // this gets a typescript error because the `this` has been cast to a default spiccato instance. You'll lose all type safety here and in some case you'll get ts compiler errors because you try to access state/getters/setters/methods that it can't confirm that it has. 
        }
    })

solution here is to remove the return type from init, getCustomGetters, getCustomSetters, etc. If you just return this, typscript can figure out what this is and you get full type safety and intellisense.

Don't want to merge this into a release branch so the NPM releases and their corresponding release branches reflect the same code. Made a new branch with your contributions and these fixes I mentioned: release-1.0.3-beta. Deployed there and that release is up on npm now too.

Thanks for adding this!