stampit-org / stamp

Stamps - better OOP model
https://stampit.js.org
MIT License
25 stars 3 forks source link

TypeScript version bump + Type enhancements #84

Open sammys opened 1 year ago

sammys commented 1 year ago

Now that this is no longer RFC I've moved this issue from #356 in stampit to here.

The new types will differentiate a generic Stamp from a Stamp that has a defined descriptor as you see in the screenshot. I've not been able to decide on a name for that differentiating type. I could do with some opinions. Here is my short list in no particular order:

  1. ComposedStamp
  2. InferableStamp
  3. DefinedStamp

Tasks

sammys commented 1 year ago

@koresar The next PR is going to have a very small breaking change in the Descriptor type.

Last time I checked, for inference to work sufficiently Typescript required readonly array types because they can be evaluated as immutable tuple types. Here is the diff of what will be required:

diff --git a/packages/compose/index.ts b/packages/compose/index.ts
index d337c18..a25b73c 100644
--- a/packages/compose/index.ts
+++ b/packages/compose/index.ts
@@ -93,9 +93,9 @@ export interface Descriptor {
   /** A set of object property descriptors (`PropertyDescriptor`) to apply to the `Stamp`. */
   staticPropertyDescriptors?: PropertyDescriptorMap;
   /** An array of functions that will run in sequence while creating an object instance from a `Stamp`. `Stamp` details and arguments get passed to initializers. */
-  initializers?: Initializer[];
+  initializers?: Initializer[] | readonly Initializer[];
   /** An array of functions that will run in sequence while creating a new `Stamp` from a list of `Composable`s. The resulting `Stamp` and the `Composable`s get passed to composers. */
-  composers?: Composer[];
+  composers?: Composer[] | readonly Composer[];
   /** A set of options made available to the `Stamp` and its initializers during object instance creation. These will be copied by assignment. */
   configuration?: PropertyMap;
   /** A set of options made available to the `Stamp` and its initializers during object instance creation. These will be deep merged. */

It might be good to release a new minor version with the dependency bumps just merged. What do you think?

koresar commented 1 year ago

@sammys I don't think this change is worth releasing at all.

Also, I think we have never shipped TS-made @stamp/ module. See for yourself: https://www.npmjs.com/package/@stamp/it?activeTab=explore

koresar commented 1 year ago

Regarding Stamp vs new Stamp situation. I think we should avoid having two different Stamp types as much as possible. Can it be one? (Either of them.)

sammys commented 1 year ago

@sammys I don't think this change is worth releasing at all.

The reasoning behind my suggestion was so packages install without having loads of dependency deprecation messages.

Also, I think we have never shipped TS-made @stamp/ module. See for yourself: https://www.npmjs.com/package/@stamp/it?activeTab=explore

That must be what you were hinting was incomplete in, if I remember correctly, the collision PR thread. Looks to be rather trivial to TS-ify @stamp/it. How do you see this playing out? I see two options:

  1. TS-ify now and release aligned packages as new minor versions before merging the inference types as new major versions
  2. Merge inference types then TS-ify and release everything as new major versions

Regarding Stamp vs new Stamp situation. I think we should avoid having two different Stamp types as much as possible. Can it be one? (Either of them.)

The main Stamp type is a base type and all stamps, inferred or legacy, will extend that type. The new type is a by-product of inference and is itself a base type. For example, consider a stamp with static properties. In this case, NewStamp does extend Stamp but Stamp does not extend NewStamp. Here is some simplified sample code that uses a TSchema generic for NewStamp and highlights what is happening.

interface Stamp extends ComposableFactory {
  compose: ComposeProperty
}

interface NewStamp<TSchema = {}> extends Stamp {
  compose: ComposeMethod & Omit<Descriptor, keyof TSchema> & TSchema
}

From the sample you can see that NewStamp is a base type for all stamps composed with a particular TSchema. This can be aliased this way:

const MySchema = {
  staticProperties: {
    hostname: "" as string
  }
}

type MySchema = typeof MySchema

type MyInferredStamp = NewStamp<MySchema>

type A = MyInferredStamp extends Stamp ? true : false
// true

type B = Stamp extends MyInferredStamp ? true : false
// false
koresar commented 1 year ago

Mate, we can break anything in this project and release as next MAJOR.

I trust you to do all the other decisions. Feel free to do whatever you want.

Do not spend time on backwards TS compatibility. It's not worth it. Feel free doing any rewrites you want.

Tell me what to do - I'll what's needed (if time permits).