Typescript: cloneNode should return sub type, not Node

[dlee]

When I do someElement.cloneNode(), I want the returned element to be the same type as someElement, but right now it's returning type Node.

[DanielRosenwasser]

I assume that you have something like the following code

var d: Document;
var x = d.cloneNode(); // 'x' is of type 'Node'

but you wanted the resulting type of d.cloneNode() to be Document as well.

This is something that cannot "automatically" be expressed by our type system (nor can it be in type systems such as Java, C#, but has been "solved" in certain other languages).

One thing we could do is make cloneNode generic so that you could do:

elem.cloneNode<Document>();

and have that return a Document - but you're not gaining much over using a type assertion.

Something that allows you to get exactly what you want would be to define your own helper function so that you generally won't have to supply the generic parameter since it can be inferred.

function cloneNode<T extends Node>(node: T) {
    return <T>node.cloneNode();
}

var d: Document;
var x = cloneNode(d); // 'x' is of type 'Document'

[fsoikin]

@DanielRosenwasser, thus can be nicely solved with generic "this"argument, as in #285.

[DanielRosenwasser]

My comment was misleading. I meant to say that there is no way to automatically indicate to the compiler that a method should return the statically known subtype of a type.

In fact, we _could_ include a signature on all subtypes of Node such that cloneNode() returns the appropriate type (we could also do certain things involving generics which would probably be a bit overkill).

[RyanCavanaugh]

Agree that this is basically the same issue as #285.

[Arnavion]

@DanielRosenwasser @RyanCavanaugh

Now that TS has this for return types, can this be done?

interface Node {
    cloneNode(deep?: boolean): this;
}

It'd technically be a lie though, since it returns a new object, not itself. Not sure if that matters.

Otherwise would it be okay to add an overload of cloneNode to every Node subtype in dom.generated.d.ts ?

[RyanCavanaugh]

I think : this should be correct here?

[DanielRosenwasser]

Wouldn't cloneNode be inherited from Node itself?

[Arnavion]

@RyanCavanaugh

The thing is : this is only really suitable for functions that really do return this, not a clone.

interface Cloneable {
    clone(): this;
}

class Foo implements Cloneable {
    clone(): this {
        // Error. Can work around by asserting to `any`,
        // but still semantically wrong if not overridden by every derived class.
        return new Foo();
    }
}

It doesn't matter as much for Node and the in-built HTML elements since they have opaque implementations of cloneNode and we can assume the browser implements them correctly. But maybe it'll matter somewhere else (custom elements?) and in general is not correct.

[xogeny]

I had a similar issue with. As I understand it, the issue here is that you cannot return anything except this because (perhaps among other things), someone could come along and do this:

class Bar extends Foo {
}

The problem with this is that then you have this problem:

  let foo = new Foo();
  let cf: Foo = foo.clone(); // As you would expect
  let bar = new Bar();
  let bf: Bar = bar.clone(); // Ack! bf is really a Foo, but the compiler thinks it is a Bar

However, I have a case where I had a fixed set of classes where I needed to do this clone operation. If you are willing to be disciplined and ensure that each subclass implements clone, I think you can safely override the compilers (correct) error message safely with a simple (if not slightly misleading) cast, i.e.,

class Foo implements Cloneable {
    clone(): this {
       return new Foo() as this; // Just hope nobody extends Foo without also adding clone.
    }
}

As I said, I think this is safe (seemed to work for me). There are semantic issues with "assignability", I didn't really dig into this to see if there are corner cases where this would break anything. But as far as I can tell, as long as you follow this discipline (always provide a clone method), you don't seem to be breaking any of the semantics.

Comments?

[mindplay-dk]

this is only really suitable for functions that really do return this, not a clone

Because this refers to the actual value of this, in the same sense that true, false or "foo" refer to actual values and not just types, right?

I think you can safely override the compilers (correct) error message safely with a simple (if not slightly misleading) cast

I don't think it would be appropriate to rely on a hack like this in the standard libraries?

Looks like what we need is something like self - a pseudo-type that statically resolves to the parent type, so that type-hinting for e.g. cloneNode() becomes possible:

interface Element {
    cloneNode(deep?: boolean): self;
}

Or with the Cloneable example:

interface Cloneable {
    clone(): self;
}

class Foo implements Cloneable {
    clone() {
       return new Foo();
    }
}

This ought to be safe, even if another class inherits that method:

class Bar extends Foo {
    // ERROR: inherited implementation of 'clone' returns 'Foo', expected 'Bar'
}

In other words, self is like a "type argument" of sorts, that always passes the current type.

Would that work??

[myfonj]

(JSDoc) Workaround / for future reference: use manual cast /** @type {name} */ (ref).

// cloneNode produces Node, @see https://github.com/Microsoft/TypeScript/issues/283
var a = document.cloneNode(true);
a.location; // Property 'location' does not exist on type 'Node'.
// workaround:
var b = /** @type {Document} */ (document.cloneNode(true));
b.location; // OK

---

Learned from #25028.

[callumlocke]

5 years old :) Is this ever going to be fixed?

[IllusionMH]

Do I understand correctly that solution which involve this like below (Playgroung link)

cloneNode<T extends Node = Node>(this: T, deep?: boolean): T;

won't be considered and expected fix is to add specific cloneNode definitions to all decedents of Node interface?

And those who create custom element will have to define their own types for this method?

[fregante]

Is it not possible to extract the class type from this? Example:

interface Node {
    cloneNode(deep?: boolean): new Constructor<this>; // Not real
}

This is the closest I got, so far:
https://www.typescriptlang.org/play/index.html#code/AQFwngDgpsDCD2A7AziATgVwMYnmgPACoB8wAvMIlAO7AAUAdEwIZoDmyAXMM4mANoBdAJTlShANwBYAFCysAG2bJkwAGLx4wAN6zg+4IqRQAcvAAmUOsO4IU6bLgIgAFgEtkpXTIO-gaKBAMNERgAHkAIwArKBwGLADmECtXD3ikVEwcPGFpHwMAX1kiuRlFZVUAIVZgKAAPZMRzVQ0tbxL5DJBgCJqKKlpqtGs8zvtKGiHyHtZ4hWMzSxGx1AnqE0m+taG5hYsrXKA

[IllusionMH]

@fregante see https://github.com/microsoft/TSJS-lib-generator/pull/302#discussion_r143547060 . It is working solution (see comment above for sorter version), but "That PR would make compile time and memory usage worse for everybody, so we can't take it." (even if they don't need cloneNode). So if you need it - add to your repo, but be aware of drawbacks.

PR that adds overrides with concrete types is in place and waiting for review.

[fregante]

So if you need it - add to your repo, but be aware of drawbacks.

Right, that's what I was looking to solve.

PR that adds overrides with concrete types is in place and waiting for review.

Oh, I see that now. Easy solution but does it work correctly on custom elements?

[IllusionMH]

does it work correctly on custom elements

No, custom elements declaration would have return type of element that they extend (which will be close enough for some cases) but need own override to have exact type :(

I'm not sure that it is possible to do so without making cloneNode generic, but it can be done on project level if needed.

[piotr-cz]

As in @myfonj example, the trick is to wrap expression in brackets ():

/** @type {HTMLTableRowElement} */
const $templateRow = document.createElement('tr')

/** @type {HTMLTableRowElement} */
const $rowClone1 = $templateRow.cloneNode(true)
// Error: `Type 'Node' is missing the following properties from type 'HTMLTableRowElement': ...`

/** @type {HTMLTableRowElement} */
const $rowClone2 = ($templateRow.cloneNode(true))
// No errors

[sandersn]

Unfortunately, we had to revert microsoft/TSJS-lib-generator#811 because it makes type parameters that extend HTMLElement invariant where they were previously covariant. See https://github.com/microsoft/TSJS-lib-generator/pull/842

@types/tablesorter is a good test case for future attempts; clone microsoft/DefinitelyTyped and then run tsc in types/tablesorter.