Definitelytyped: @types/jquery Incorrect Core Type Information

[Steve-Fenton]

If you know how to fix the issue, make a pull request instead.

• [x] I tried using the @types/jquery package and had problems
• [x] I tried using the latest stable version of tsc. https://www.npmjs.com/package/typescript
• [x] I have a question that is inappropriate for StackOverflow

@leonard-thieu @borisyankov @choffmeister @Diullei @tasoili @jasons-novaleaf @seanski @Guuz @ksummerlin @basarat @nwolverson
@derekcicerone @AndrewGaspar @seikichi @benjaminjackman @s093294 @JoshStrobl @johnnyreilly @DickvdBrink @King2500 @terrymun @martin-badin

Summary of the Problem

We found this while looking at Martin Badin's fix for a bug with the "find" method, which was returning the wrong type.

More information here: https://www.stevefenton.co.uk/2020/04/jquery-and-typescript-we-have-a-big-problem-with-jquery-d-ts/

The following core type constraint for jQuery is wrong:

interface JQuery<TElement = HTMLElement> extends Iterable<TElement> {
    //...
}

It ought to be:

interface JQuery<TElement = Element> extends Iterable<TElement> {
    //...
}

This is because other sub-classes of Element are not substitutable for HTMLElement and people use jQuery with these other sub-classes (i.e. SVGElement).

Example where there _should_ be a warning, but isn't:

const svgPathElement: JQuery<HTMLElement> = $('path');

Why Haven't I Submitted a Pull Request

Fixing this will potentially generate a lot of warnings in codebases that happen to only use HTMLElement and have accidentally become dependant on the incorrect assumption. The cases where the assumption _is_ violated (and therefore are likely to hit real bugs that the type system doesn't warn them) are probably smaller than the number of cases where the assumption matches most, or all, of the implementation.

Before we fix this, I think we need to hear arguments for or against fixing it.

[leonard-thieu]

When I authored the definitions, I set HTMLElement as the default type mostly for usability reasons. The majority of users will be using jQuery on HTML. If Element is the default, they'll end up having to declare HTMLElement in a number of scenarios. Also, at the time, @types/jquery@2 used HTMLElement as the core type. It seemed like a good idea to match it to reduce the effort needed to go into migration,. @types/jquery@2 has since moved to using Element.

Changing the core type to Element makes the experience worse for HTML users and I'm not sure it necessarily improves the situation for SVG users. Currently, SVG users have to assert that they're working with SVG elements. If the core type is changed to Element, does this actually change for them? Do they end up using properties exclusive to Element or will they still have to assert an SVG element type? There's also the issue of SVG users having to _know_ that they must assert an SVG element type. I know that this is a big blind spot when using HTMLElement as the default core type.

I'm not really a big fan of either solution. I'd prefer one that takes both usability and correctness in mind. If I had to pick between the 2 solutions, I'd prefer keeping the current one.

[guardiannite]

I'm not sure if it's really worth me posting this. But I was going to submit an issue and found out it matches this issue.

Previous:

let newDot = $('<div class="dot-spacer"><div class="dot"></div></div>');
.
.
newDot = newDot.find('.dot');

Now:

Error TS2322: Build:Type 'JQuery<Element>' is not assignable to type 'JQuery<HTMLElement>'.

So for the time being, I just removed my floating version dependency of "@types/jquery": "^3.3.29" to "@types/jquery": "3.3.29"

[wmorrell]

Something like this broke between @types/[email protected] and @types/[email protected], so it may be worth trying to pin to 3.3.35 instead of 3.3.29.

Looks like #43857 may be the cause of code breaking? I'm seeing issues where using JQuery.each and creating a new JQuery object from the Element argument is resulting in type mismatches on methods as above, Type 'JQuery<Element>' is not assignable to type 'JQuery<HTMLElement>'. Note, this is generating errors, not warnings.

[Steve-Fenton]

Thank you for your information on these errors. It's because the .find method is "correct" and the JQuery interface contains the assumption of HTML.

If the outcome of this discussion is to leave JQuery pinned to HTML we will have to update .find to the same assumption.

[wmorrell]

Linked to the wrong PR, it's #44051.

As someone who is using the JQuery types for mostly HTML, with a very little bit of SVG, I am slightly biased toward keeping the assumption of HTML out of laziness and wanting to avoid doing casts though large parts of code, instead of only for the bits that deal with SVG. But I also see the point of fixing it to truly be correct. I'm only seeing a few dozens of errors on 3.3.36 with a several thousands of lines codebase, so the pain of fixing isn't that bad and has potential to catch some errors like in the OP. It's a little surprising to have the sudden breakage in a 'minor' release, but that might be unavoidable to keep tracking the actual JQuery version.

Is there a prominent place to document that pinning to 3.3.35 is a quick-fix for those on a deadline?

[martin-badin]

@wmorrell the PR is not wrong. The PR fixing a case when a user wants to use an element typeof Element. For example document.activeElement is typeof Element and not HTMLElement.
https://www.typescriptlang.org/play/?ssl=1&ssc=1&pln=1&pc=34#code/MYewdgzgLgBApgGxgXhgExMArgWzmKAOgENgoBLANzgFEE48Cg

[wmorrell]

@martin-badin I didn't say the PR is wrong. I did say that it's a breaking change, in that code that worked fine using @types/[email protected] is now generating errors when using @types/[email protected]. The purpose of this thread, as I understand it, is to get feedback on whether it makes sense to do these breaking changes to make the typings "correct", or to continue with the early authors' choice to have the types use HTMLElement instead of Element for compatibility with @types/jquery@2.

Arguments about the "correctness" of the PR are really irrelevant to the discussion. It's obviously "correct", in the sense that the real types are Element instead of HTMLElement. It also breaks existing code, and requires littering existing code with casts and generics, even when that code deals exclusively with HTMLElement and functions flawlessly when all typing information is removed. Is it really better to force code to change $(selector1).find(selector2) to read $(selector1).find<HTMLElement>(selector2)? Or is the status quo ex ante of only requiring generic defs for things like $(selector1).find<SVGElement>('path'), while allowing bare find "OK", even if it's technically "wrong"? Do you think that everyone should update their code to read as$(selector1).find<HTMLElement>(selector2)?

[martin-badin]

I agree with you. I don't think that everyone should update their code. I'll wait for the rest of the users.

I can create PR to fix return type to HTMLElement if a user passes Element as the param for now.

...find<E extends HTMLElement>(selector_element: JQuery.Selector | Element | E | JQuery<E>): JQuery<E>;

[vsajip]

Perhaps it would make sense to switch it back to HTMLElement for now, so that the current breakages in client code go away. There will be a lot of users who don't care about the distinction because they don't use other element types a lot of the time - they should not necessarily feel the pain for things they don't use. While I agree that Element is correct and the type system should help find problems, perhaps that level of care about correctness should be opt-in. After all, TypeScript allows you to loosen type constraints where it's practical to do so; this might be a case of "practicality beats purity". The correctness can always be reintroduced later, once a less painful way of introducing the change is found. Just my 2¢ - I'm fairly new to TypeScript and want to introduce it gradually into existing codebases.

[misium]

(Edit: I lied 3.3.35 is fine, I didn't wait long enough for typings to resolve. Hopefully this isn't the start of a flood of issue tickets -- looks like the \

Woke up to a host of typescript errors this morning. Rolling back to 3.3.35 didn't work for me, had to go back to 3.3.34.

Read the debate, you guys are asking the right questions, no obvious answer here.

While y'all are debating please make a clear note that rollbacks to 3.3.34 or .35 are the quick fix here. Will save everyone some frustration.

[HolgerJeromin]

I found this related problem with the latest master:

        let child = $(document.body.firstElementChild);
        let container = $<Element>().add(child);

The add does not allow an Element-jQuery object.

[Steve-Fenton]

@HolgerJeromin

Do you need to explicitly pass the Element type as a type argument? How would your code suffer if you removed that and had an HTMLElement rather than an Element?

(Additional note: this is a genuine open question as it will help people understand which direction to take... there is no fix that will make things work for both the HTMLElement camp _and_ the Element camp)

[martin-badin]

I think that the example is wrong because we can't pass Element as a type to the JQuery object. JQuery object accepts only HTMLElement and a type extended from HTMLElement.

$<Element>()...

[HolgerJeromin]

I think that the example is wrong because we can't pass Element as a type to the JQuery object. JQuery object accepts only HTMLElement and a type extended from HTMLElement.

Then we would have to change the typings:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/106c646cc7cf1caa8e291d3a15401083e80ffb46/types/jquery/JQueryStatic.d.ts#L170

jQuery as a container format works without problems with SVGElement. Many manipulations are possible without problems, too.

[martin-badin]

I see, thanks. JQuery object accepts whatever.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/106c646cc7cf1caa8e291d3a15401083e80ffb46/types/jquery/JQuery.d.ts#L5

Only the add function accepts JQuery<HTMLElement> and types extended from HTMLElement but not Element.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/106c646cc7cf1caa8e291d3a15401083e80ffb46/types/jquery/JQuery.d.ts#L230

We could change it to:

add(selector_elements_html_selection: JQuery.Selector | JQuery.TypeOrArray<Element> | JQuery.htmlString | JQuery<Element> | JQuery.Node): this;

or you can use:

let container = $<Element>().add(document.body.firstElementChild);