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

Don't use api.jquery.com as source for definition typings #1499

Closed
nvivo opened this Issue Jan 3, 2014 · 5 comments

Comments

Projects
None yet
4 participants
@nvivo
Copy link
Contributor

nvivo commented Jan 3, 2014

I noticed that jquery definition is adding lots of documentation to the d.ts which is good, but it is also using type information from the documentation which is terrible.

api.jquery.com is full of type errors, and is just not reliable. Lots of places (like the documentation for .html()) will say it accepts a string when it actually accepts anything that jquery constructor accepts, like elements, other jquery objects, etc.

Although api.jquery.com is official, it was not created to give accurate typing information, so using that is just breaking a lot of existing and correct code.

If that is going to be used, it needs to be checked, as the most accurate type for most parameters will be actually "any".

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Jan 3, 2014

Hi @nvivo

Hope you had a good Christmas and New Year! I wanted to respond to this issue and #1500 as well.

I've looked at the code behind html in the jQuery source and you're right that jQuery uses this.empty().append( value ); under the covers. However, this is only used if a potential elem.innerHTML = value; has try / catch-ed earlier on. So what you say doesn't follow in all cases.

But perhaps more to the point - what's happening with this.empty().append( value ); and elem.innerHTML = value; is essentially implementation detail. It could theoretically change with the next patch of jQuery to a different implementation. Unlikely I know but hopefully you see my point? What I'm saying is that it's probably best to set our typings to follow the official API rather than create our own which may later become invalid. And for my money typings of any don't give us a great deal of benefit.

If there isn't a better source of API documentation available I think it's probably best to stick with what's listed on http://api.jquery.com . I don't know if there is a better source of documentation available that you know of? Certainly from reading the jQuery source it's not always obvious what all the available overloads are. I had to hunt high and low for the function overload of html!

By the way, if you think that the jQuery API is invalid is it straightforward to submit a pull request to change the documentation? I seem to recall that the jQuery validation documentation is on GitHub so it's possible that jQuery will also be....

@nvivo

This comment has been minimized.

Copy link
Contributor

nvivo commented Jan 3, 2014

Hey John,

api.jquery.com was not created to give correct typing information. To follow api.jquery.com blindly for typing information will just produce incorrect definition that will be useless.

I won't start submitting PRs for jquery api because that will be an endless job syncing the types. Type information required by typescript is exclusive to typescript, not to jquery. Their documentation is not even prepared to map things the way TS requires.

The implementation detail is actually not important as you said, but the fact that .html() accepts other jquery objects, or dom elements is. That is existing behavior that won't change, and people depend on it.

I'm not saying to ignore api.jquery.com completely, just don't use their type information as absolute truth because their documentation just doesn't care that much for its correctness.

@basarat

This comment has been minimized.

Copy link
Member

basarat commented Jan 4, 2014

For reference we are talking about this section of code : https://github.com/jquery/jquery/blob/1.9-stable/src/manipulation.js#L217-L257

is essentially implementation detail. It could theoretically change with the next patch of jQuery to a different implementation

Yes. But unlikely as it would cause other people's code to break.

And for my money typings of any don't give us a great deal of benefit.

Possible suggestion use the same signature for html as you have for append http://api.jquery.com/append/ htmlString or Element or Array or jQuery That would prevent having an any and be semantically correct.

If there isn't a better source of API documentation available I think it's probably best to stick with what's listed on http://api.jquery.com .

Agreed.

just don't use their type information as absolute truth because their documentation just doesn't care that much for its correctness.

It does. But we as programmers abuse the underlying implementation which is a fair conscious choice to make. Following the docs would be recommended but since you (and possibly others) have code that depends on this signature of html we can make an exception.

@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Jan 4, 2014

What is wrong with using the documented interface? @johnnyreilly is right that it's implemention detail.

Yes. But unlikely as it would cause other people's code to break.

That is exactly why code does break, because people use undocumented interfaces.

@basarat

This comment has been minimized.

Copy link
Member

basarat commented Jan 4, 2014

@dmethvin Thanks for the confirmation. That tweet was specifically to get someone like you interested.

@nvivo guess we have our answer from a member of the jquery team 👍 @johnnyreilly is correct

@basarat basarat closed this Jan 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment