type issue: `current` of Draft<T> type should return T type

[francescotescari]

The following TS code doesn't compile, unexpectedly, due to the fact that current(Draft<T>) returns Draft<T> instead of T. Can we make the return type of current(Draft<T>) T? Or even force current to take Draft<T> as input arg (given that there is also a runtime check preventing non-draft objects)?

  function test<T extends { x: { y: ReadonlySet<string> } }>(base: T): T {
    const [draft] = create(base);
    const currentValue: T = current(draft); // Type Draft<T> is not assignable to type T
    return currentValue;
  }

As a separate typing issue, it would be nice if Draft<T> could be assigned to T, which is not the case currently.

  function test<T extends { x: { y: ReadonlySet<string> } }>(base: T): T {
    const [draft] = create(base);
    const currentValue: T = draft; // Type Draft<T> is not assignable to type T
    return currentValue;
  }

In our use case it would be handy because we have an opaque, generic mutating function (e.g. mutate(obj: T)) and we want to pass a Draft<T> to such function. However we have to cast Draft<T> to T to do that, even if Draft<T> is conceptually a subtype of T. In practice I didn't find a way to do this due to T could be instantiated with an arbitrary type which could be unrelated to Draft<T>

[unadlib]

1. Regarding the return type of current(Draft<T>): Indeed, changing the return type to T instead of Draft<T> is more reasonable. The purpose of the current() function is to get the actual value of the current draft, so returning T is more semantically appropriate. I would modify the type definition of current() to:

function current<T>(value: Draft<T>): T;

2. Regarding Draft<T> not being assignable to T: This issue is more complex. From a type safety perspective, Draft<T> indeed should not be directly assigned to T, as the draft might contain some intermediate states. However, to increase flexibility, we could consider adding a new type conversion function, such as castMutable:

function castMutable<T>(draft: Draft<T>): T;

This function should not perform any operations at runtime, it's only used for type conversion. When using it:

function test<T extends { x: { y: ReadonlySet<string> } }>(base: T): T {
  const [draft] = create(base);
  const mutableValue: T = castMutable(draft); 
  return mutableValue;
}

This allows converting Draft<T> to T where needed, while clearly indicating through the function name that this is a potentially unsafe operation. From a typing perspective, we don't recommend using castMutable to convert to T, as T might typically be a deeply readonly type or a common type.

---

Furthermore, modifying the generic type of current() is a breaking change. I'm considering whether to introduce a new API, for example, getCurrent().

[unadlib]

I'm not sure if this change meets your requirements. We also welcome others to share their opinions on this issue.

[francescotescari]

1. Regarding the return type of current(Draft<T>)

Agree

2. From a type safety perspective, Draft<T> indeed should not be directly assigned to T, as the draft might contain some intermediate states.

But all the intermediate state are all of type T. Draft<T> is just a deeply mutable (non-readonly) version of T, isn't in? In the same way you can assign an Array<Array<number> to a ReadonlyArray<ReadonlyArray<number>>, the same should be for Draft<T> assignable to T, without safety issues (assuming T can describe at most a readonly type and never immutability). I didn't find a nice typing solution either, so the cast function solution sounds good to me.

Furthermore, modifying the generic type of current() is a breaking change. I'm considering whether to introduce a new API, for example, getCurrent().

Since it's just about types and no runtime changes, what about adding an overload like

/**
[...]
 *   },
 * );
 * `
 */
export function current<T extends object>(target: Draft<T>): T;
/** @deprecated You should call current only on `Draft<T>` types. */
export function current<T extends object>(target: T): T;
export function current<T extends object>(target: T | Draft<T>): T {
  if (!isDraft(target)) {
    throw new Error(`current() is only used for Draft, parameter: ${target}`);
  }
  return getCurrent(target);
}

[unadlib]

Since it's just about types and no runtime changes, what about adding an overload like

From a type-checking perspective, this is still a breaking change.

[unadlib]

hi @francescotescari , thanks for your suggestion. I've released the Mutative v1.1.0.