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

Suggestion: introduce UniveralElement to eliminate most type assertions in DOM programming (prototype created) #3304

Closed
duanyao opened this issue May 29, 2015 · 11 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@duanyao
Copy link

duanyao commented May 29, 2015

TS heavily relies on type assertion in DOM programming. In my experiences, type assertion is a major factor of produtivity-reduction in font-end TS. There is another user expressed similar concern (#3263).

So I try to eliminate most type assertions in DOM programming by adding a UniveralElement interface to lib.d.ts, see my fork of lib.es6.d.ts.

In my fork, querySelector(), getElementById(), getElementsByClassName(), children, firstElementChild etc. return UniversalElement or a list of UniversalElement, which inherits all known sub interfaces of Element; UIEvent::target is typed as UniversalUIEventTarget, which extends UniversalElement, Document, and Window. So in most case so you don't have to cast the result before use anymore. Additionally you don't loose the power of type check and autocompletion.

A snapshot of autocompletion using my prototype:

And some sample codes:

/// <reference path="lib.es6.d.ts" />

var ue = document.querySelector('#foo'); // UniversalElement
// from Element
var cls: string = ue.className;
// from HTMLElement
ue.blur();
// from HTMLMediaElement
ue.play();

// from HTMLInputElement
var files = document.querySelectorAll('input[type=file]')[0].files;
var fname = files[0].name;

// from HTMLImageElement
var nh = document.body.children[0].naturalHeight;

var svg = document.getElementsByClassName("bar")[0];
// from SVGElement
var vp = svg.viewportElement;
// from SVGLineElement
svg.x1.baseVal.newValueSpecifiedUnits(1, SVGLength.SVG_LENGTHTYPE_PX);

// type of href from HTMLAnchorElement becomes "any" because of type confliction
// with SVGElements
var href = document.querySelector('a').href;

// UIEvent::target is UniversalUIEventTarget, which extends UniversalElement,
// Document, and Window
document.addEventListener('mousemove', ev => {
    var targ = ev.target;
    if (targ.classList.contains('movable')) {
        targ.style.left = ev.pageX + 'px';
        targ.style.right = ev.pageX + 'px';
    }
});

@NekR and @kitsonk, what do you think?

@mhegazy
Copy link
Contributor

mhegazy commented May 29, 2015

The problem is these typings are not true. These typings claim that document.querySelectorAll will return something that is both an HTMLElement and an SVGElement; where the reality is it returns either an HTMLElement or an SVGElement.

The reason why you need to cast is not because the typings are not correct, but because the DOM structure, that exist at run-time, does not treat SVGElement as an HTMLElement, and there is nothing we can do about that. faking the typings to say all elements have all properties makes you loose type safety, and forces users to check at runtime whether these properties exist or not before using them, which is probably the main reason you would use TypeScript.

I understand that SVGElements are not common vs. HTMLElement, but they do exist.

For your cases, you can always pass --noLib, provide your version with the library, where you have changed the definition of queryselector to always return an HTMLElement, or any for that matter, and the compiler will get out of your way. it is your responsibility though to make sure that this assumption will not bite you back when your code runs on a DOM tree with an SVGElement.

@duanyao
Copy link
Author

duanyao commented May 30, 2015

faking the typings to say all elements have all properties makes you loose type safety, and forces users to check at runtime whether these properties exist or not before using them, which is probably the main reason you would use TypeScript.

This is not the case. Previously, we usually write code like this:

(<HTMLInputElement>document.querySelector('input[type=file]')).files[0]...

But how does TS comipler know the return value of querySelector() is actually a HTMLInputElement or not? Of cause it doesn't know, instead it have to trust the users not shooting themselves in the foot. So type assertion doesn't give us extra type safety at all in such use case.

In most cases, users are sure about what type of Element a perticular querySelector() call will return at runtime, so no runtime checking of the return type is needed.

If the actual return type is uncertain, we have to do runtime checking anyway, and nothing TS can do for us. Previously we do it like this:

var foo = document.querySelector('#foo');
if ((<HTMLInputElement>foo).files) {
  let fileInput = <HTMLInputElement>foo;
  fileInput.files[0]...
}

Do you see a small issue here? Before we can check the existance of .files in foo, we have to pretend it is a HTMLInputElement, so this is "the chicken or the egg" paradox. Maybe someone would write if ((<any>foo).files) instead, just by-pass type checking of TS. In other strongly typed languages, code like if ((<HTMLInputElement>foo).files) is either true or throws an runtime error. So type assertion is not quite benificial for runtime checking too.

On the other hand, with UniversalElement, we do it like this:

var foo = document.querySelector('#foo');
if (foo.files) {
  let fileInput = <HTMLInputElement>foo;
  fileInput.files[0]...
}

Or simply

var foo = document.querySelector('#foo');
if (foo.files) {
  foo.files[0]...
}

It is equally type-safe, and more natural to read in my opinion. Of cause if we can mark .files etc. as optional in UniversalElement things will be more logical, but currently there is no convinient way to optionally inherit a interface, i.e. we can't do

interface UniversalElement extends Element, HTMLInputElement?, ..., SVGLineElement? {
}

So users need to know that .files etc. do not always exist in UniversalElement. This is not a big issue with a good IDE, because the IDE can tell you that .files is defined in HTMLInputElement, not in Element.

I understand that SVGElements are not common vs. HTMLElement, but they do exist.

UniversalElement also extends all SVG*Element interfaces, so it works equally well for SVG.

Hope I have made myself clear.

@jeffreymorlan
Copy link
Contributor

I grepped the source of the codebase I've been working on and found about one <HTML*Element> cast per 900 lines of code on average.

If you have a lot of casts, this may be due to programming in a type-unfriendly style. Here's a simple example of a class that uses the DOM:

class Foo1 {
    private main: HTMLElement;
    constructor(parent: Node, leftText: string, rightText: string) {
        parent.appendChild(this.main = document.createElement('div'));
        var left = this.main.appendChild(document.createElement('span'));
        left.textContent = leftText;
        var right = this.main.appendChild(document.createElement('span'));
        right.textContent = rightText;
    }
    setLeftColor(color: string) { (<HTMLElement>this.main.children[0]).style.color = color; }
    setRightColor(color: string) { (<HTMLElement>this.main.children[1]).style.color = color; }
}
var f1 = new Foo1(document.body, 'left', 'right');
f1.setLeftColor('blue'); f1.setRightColor('green');

Here we've thrown away the references to the left and right nodes, so in the setLeftColor/setRightColor methods we have to dig them out of the DOM and are forced to cast from Element to HTMLElement. But that's not the only way this class could be written. All we have to do is save those references:

class Foo2 {
    private left: HTMLElement;
    private right: HTMLElement;
    constructor(parent: Node, leftText: string, rightText: string) {
        var main = parent.appendChild(document.createElement('div'));
        main.appendChild(this.left = document.createElement('span'));
        this.left.textContent = leftText;
        main.appendChild(this.right = document.createElement('span'));
        this.right.textContent = rightText;
    }
    setLeftColor(color: string) { this.left.style.color = color; }
    setRightColor(color: string) { this.right.style.color = color; }
}
var f2 = new Foo2(document.body, 'left', 'right');
f2.setLeftColor('blue'); f2.setRightColor('green');

No casts needed. The code is also more robust: suppose we add a third element between left and right; that would break Foo1 (as main.children[1] would now refer to the middle element) but not Foo2.

I'd say that frequent use of .querySelector, .children, and so on in TypeScript should be considered an anti-pattern - good TypeScript code tends to treat the DOM structure as write-only for the most part.

@duanyao
Copy link
Author

duanyao commented May 30, 2015

I'd say that frequent use of .querySelector, .children, and so on in TypeScript should be considered an anti-pattern - good TypeScript code tends to treat the DOM structure as write-only for the most part.

It depends on what type of code you are writing. If you are using a UI framework that generates all DOM on the fly, then you are right (and I'm actually using one in my project). But if you are writing codes in progress enhancement way, or codes manipulating free format document (e.g. a rich text editor), then there are plenty of valid use cases of .querySelector() and .children.

Also note that we almost always need to cast Event::target if you want to do something meaningful with it. See my updated sample code above.

@NekR
Copy link

NekR commented May 30, 2015

For your cases, you can always pass --noLib, provide your version with the library, where you have changed the definition of queryselector to always return an HTMLElement, or any for that matter, and the compiler will get out of your way

This really does not seems right to me because I already has lib.d.ts from master since on npm it misses some thing (or at least missed) + my own playfrom.d.ts which includes things missed from IE such as Element.remove() or CSSStyleDeclaration.willChange, and many other things.

it is your responsibility though to make sure that this assumption will not bite you back when your code runs on a DOM tree with an SVGElement.

This anyway requires developer knowledge about SVGElement in tree, and it anyway requires cast. That said, querySelector is not usable at all without cast, in 99% cases you need to cast it, whatever it's html or svg element. So here compiler anyway relies on developer assumption about that element is svg or html. Also, as mentioned in earlier messages, if you are developing complex app/project with a lot of direct dom interactions you will probably to check type of Element at runtime.

I'd say that frequent use of .querySelector, .children, and so on in TypeScript should be considered an anti-pattern - good TypeScript code tends to treat the DOM structure as write-only for the most part.

Why? It's still JS and web platform. TypeScript does not limits your to any specific development or use cases. You still can and should be able to develop with it whatever you can with JS.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 10, 2015

The compiler does not treat the library any differently than any other file. --noLib is a supported way of using a custom library file. If you see this suggestion as an appropriate way to type DOM for your scenarios, by all means go for it, and the compiler should just not give you any more hard time. For reasons outlined previously in #3304 (comment) I do not believe this is a general solution that can be incorporated in the default library.

@mhegazy mhegazy closed this as completed Jun 10, 2015
@mhegazy mhegazy added Suggestion An idea for TypeScript Declined The issue was declined as something which matches the TypeScript vision labels Jun 10, 2015
@duanyao
Copy link
Author

duanyao commented Jun 10, 2015

@mhegazy I think I've answered your concern above ( #3304 (comment) ), maybe you are too busy to reply in detail.

I'd like to stress:

  • UniversalElement approch is as safe(or unsafe) as type assertion used today in DOM programming.
  • WebIDL of DOM should not always be the consitution of TS typing. WebIDL is designed mainly for the purpose writting specs, and is not always suitable for actual programming. DOM spec also has many issues itself. E.g. there is interface HTMLSpanElement, but no HTMLSectionElement; all instances of Element have style property, but Element interface doesn't.
  • Currently lib.d.ts has already some patches to WebIDL of DOM for convenience, E.g. getElementById() returns HTMLElement instead of Element per spec. I don't see why UniversalElement is much different.

@NoelAbrahams
Copy link

@duanyao, I would follow the advice of @jeffreymorlan above and encapsulate the interaction with the DOM. (I just did a search of our large code-base and we have just 3 references to querySelector, and 22 references to `getElementById, but 21 of those 22 references were in the test harness.)

It doesn't make sense to me to define this composite interface called UniversalElement that is going to be something very TypeScript-specific and add another level of indirection simply to solve a rather contrived problem.

@duanyao
Copy link
Author

duanyao commented Jun 10, 2015

@NoelAbrahams If your project rarely uses querySelector/getElementById etc., it is just fine.
But this is not the case for many other projects (including mine). The popular library jQuery was born to provide such "query" functionality and later implemented by querySelector.

I'm not sure what do you mean by "TypeScript-specific" and "indirection". Actually I think UniversalElement makes client code looks more like plain JS, because no casts required. Valid JS code should be valid TS code -- but this is usually not true for codes that use querySelector etc., unless something like UniversalElement is introduced.

@NoelAbrahams
Copy link

JQuery is an anti-pattern 😄 and mimicking that behaviour is not entirely advisable. "TypeScript-specific" and "indirection" because we are attempting to replace well-known interfaces with something new.

@duanyao
Copy link
Author

duanyao commented Jun 10, 2015

@NoelAbrahams ,
So you are saying querySelector is also an anti-pattern and WHATWG/W3C made a big mistake to introduce it? In my experience, querySelector or similar APIs can only be avoided if we are using a comprehensive web framework, but this is not always feasible.

I don't think most web developers are familiar with all those DOM interfaces, because they don't use them directly in JS. How do you know you need a cast or not? You have to know in which interface a property is defined first -- this is a big burden.

In contrast, with UniversalElement, developers don't have to remember those details, IDE can help them. Additionally, the client code is very similar to its equivalent JS code, so I'd say UniversalElement make the code less "TypeScript-specific", not more.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants