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

TypeScript version bump + Type enhancements #84

Open
2 of 4 tasks
sammys opened this issue Jan 10, 2023 · 5 comments
Open
2 of 4 tasks

TypeScript version bump + Type enhancements #84

sammys opened this issue Jan 10, 2023 · 5 comments

Comments

@sammys
Copy link
Contributor

sammys commented Jan 10, 2023

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
Copy link
Contributor Author

sammys commented Jan 10, 2023

@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
Copy link
Member

koresar commented Jan 10, 2023

@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
Copy link
Member

koresar commented Jan 10, 2023

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
Copy link
Contributor Author

sammys commented Jan 14, 2023

@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
Copy link
Member

koresar commented Jan 16, 2023

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).

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

2 participants