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

cloneNode should return sub type, not Node #283

Open
dlee opened this issue Jul 28, 2014 · 23 comments · Fixed by microsoft/TypeScript-DOM-lib-generator#811
Open

cloneNode should return sub type, not Node #283

dlee opened this issue Jul 28, 2014 · 23 comments · Fixed by microsoft/TypeScript-DOM-lib-generator#811
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Milestone

Comments

@dlee
Copy link

dlee commented Jul 28, 2014

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

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

fsoikin commented Jul 28, 2014

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

@DanielRosenwasser
Copy link
Member

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

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

@Arnavion
Copy link
Contributor

Arnavion commented Mar 8, 2016

@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 RyanCavanaugh added Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript and removed Duplicate An existing issue was already created labels Mar 9, 2016
@RyanCavanaugh
Copy link
Member

I think : this should be correct here?

@RyanCavanaugh RyanCavanaugh reopened this Mar 9, 2016
@DanielRosenwasser
Copy link
Member

Wouldn't cloneNode be inherited from Node itself?

@Arnavion
Copy link
Contributor

Arnavion commented Mar 9, 2016

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

@mhegazy mhegazy added Bug A bug in TypeScript Help Wanted You can do this labels Mar 29, 2016
@mhegazy mhegazy added this to the Community milestone Mar 29, 2016
@xogeny
Copy link

xogeny commented Nov 13, 2016

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

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

myfonj commented Mar 4, 2019

(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
Copy link

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

@IllusionMH
Copy link
Contributor

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

fregante commented Jan 6, 2020

@IllusionMH
Copy link
Contributor

@fregante see microsoft/TypeScript-DOM-lib-generator#302 (comment) . 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
Copy link

fregante commented Jan 6, 2020

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

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

piotr-cz commented Mar 9, 2020

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

sandersn commented Apr 2, 2020

Unfortunately, we had to revert microsoft/TypeScript-DOM-lib-generator#811 because it makes type parameters that extend HTMLElement invariant where they were previously covariant. See microsoft/TypeScript-DOM-lib-generator#842

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

@clshortfuse
Copy link

This works:

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

Not sure if this wasn't supported before, but seems fine now. DocumentFragment and Element and Node all clone their respective types. Even mixed typed (eg: (DocumentFragment|Element).

The only problem I see is ChildNode.cloneNode() should return Node, though, there's a probably a way to coerce that in the ChildNode interface.

@HolgerJeromin
Copy link
Contributor

@clshortfuse
This has been rejected 5 years ago (for performance reason ?):
microsoft/TypeScript-DOM-lib-generator#302 (comment)
#17818 (comment)

@clshortfuse
Copy link

There's also typeof this which sounds the same, but isn't. (See #51265)

There's not much information about the performance drain, so I can't speak to if typeof this would be any different.

@callionica
Copy link

callionica commented Mar 29, 2024

This continues to be a problem. Could someone reassess fixing this? If there are still problems with more complete solutions, please add a specific version of cloneNode for Element and HTMLElement. If cloneNode returned Element from an Element (even if no changes for more specific types), it would still fix a bunch of code and be a huge usability improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.