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

jQuery: continuing JSDoc #1835

Merged
merged 2 commits into from Mar 13, 2014

Conversation

Projects
None yet
3 participants
@johnnyreilly
Copy link
Member

johnnyreilly commented Mar 13, 2014

Introducing missing test suites / tightening up typings along the way.

This PR could prove controversial as part of it makes the simple text setter API compliant. i.e. It goes from this:

text(textString: any): JQuery;

to this:

text(textString: string): JQuery;

as per http://api.jquery.com/text/#text-textString

However, the jQuery documentation is littered with examples of passing numbers / booleans etc to text instead of strings. Some examples:

This makes me think that the examples in the jQuery documentation are wrong or that the jQuery API documentation is wrong. I don't know which.

@dmethvin could you pitch in here perhaps?

jQuery: continuing JSDoc
Introducing missing test suites / tightening up typings along the way
@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Mar 13, 2014

There are a lot of places where we delegate to native APIs and things just happen to work. For .text(arg) it calls document.createTextNode(arg) which does work if arg is numeric. Of course after several years there will be plenty of code out there depending on this behavior so the documentation would need to change. You're welcome to file pull requests at https://github.com/jquery/api.jquery.com/ for anything you see, we'd love to get some help cleaning these things up.

@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Mar 13, 2014

If it helps, I'd say that .text() requires a primitive type. People might pass in a Date but that format varies by browser so it's not very useful for production. The native API would stringify anything I suspect but [object Object] is a pretty useless result.

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Mar 13, 2014

Thanks for responding @dmethvin - I'll change my PR to allow string / boolean / number based on your input.

If I get some time I'll try and file a PR at https://github.com/jquery/api.jquery.com/ too.

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Mar 13, 2014

I've amended this PR and raised a separate PR to change the jQuery API documentation as discussed.

johnnyreilly added a commit that referenced this pull request Mar 13, 2014

Merge pull request #1835 from johnnyreilly/master
jQuery: continuing JSDoc

@johnnyreilly johnnyreilly merged commit 7a0963c into DefinitelyTyped:master Mar 13, 2014

1 check passed

default The Travis CI build passed
Details
@niemyjski

This comment has been minimized.

Copy link
Contributor

niemyjski commented Mar 17, 2014

Can we also accept dates?

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Mar 17, 2014

I suspect not given that at present Date is not part of the officially supported API and given the comment by @dmethvin specifically about Date I don't think it will come to be either.

Will close the issue as I don't think the situation will change.

@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Mar 17, 2014

It wouldn't be very cross-platform to say .text(new Date()) since each browser stringifies that differently and it would be useless for anything but visual inspection. There are plenty of ways to get a date string in a known and/or localized format in JavaScript so that would be the way to go.

@niemyjski

This comment has been minimized.

Copy link
Contributor

niemyjski commented Mar 17, 2014

Yeah, I get that.. but its a valid JavaScript type.. I don't really care
how it's formatted, I just want it to be represented as a fallback text
value.

Thanks
-Blake Niemyjski

On Mon, Mar 17, 2014 at 4:14 PM, Dave Methvin notifications@github.comwrote:

It wouldn't be very cross-platform to say .text(new Date()) since each
browser stringifies that differently and it would be useless for anything
but visual inspection. There are plenty of ways to get a date string in a
known and/or localized format in JavaScript so that would be the way to go.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1835#issuecomment-37871034
.

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Mar 17, 2014

I'd advise JSON.stringify for your serialisation needs in that case. That'll give you a string you can pass to text.

@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Mar 17, 2014

Seems like passing in arbitrary stuff is defeating the intent of strict typing. In a debugging scenario you can just say .text(String(unknownArbitraryStuff)) or .text(""+unknownArbitraryStuff) and be pretty sure something will appear as long as it has a .toString method of some kind. The slight additional verbiage is good because then you can explain to someone later why you didn't mind that there was some arbitrarily-formatted string returned depending on the browser.

@niemyjski

This comment has been minimized.

Copy link
Contributor

niemyjski commented Mar 17, 2014

It's a instance of a date not some random object... Sigh... I'll call toString() on it, it just kind of sucks that date isn't handled by jquery/this typing.. If someone is going to complain about the output of a date object then they should have formatted it before passing it to text().

Thanks
-Blake Niemyjski

On Mon, Mar 17, 2014 at 4:23 PM, John Reilly notifications@github.comwrote:

I'd advise JSON.stringify for your serialisation needs in that case.
That'll give you a string you can pass to text.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1835#issuecomment-37872418
.

@dmethvin

This comment has been minimized.

Copy link

dmethvin commented Mar 17, 2014

.text(/Well then this should be allowed as well since it's a builtin type/)

@niemyjski

This comment has been minimized.

Copy link
Contributor

niemyjski commented Mar 17, 2014

@dmethvin Why stop at date... I think we should remove the overloads for number and boolean. /rant

@johnnyreilly

This comment has been minimized.

Copy link
Member

johnnyreilly commented Mar 18, 2014

A thought: whilst the Date text overload won't be going into the DT jQuery typings you are free to add that overload in a separate file if that makes your life easier. It's obviously not officially supported but that deals with your particular use case and should still continue to work even when jQuery.d.ts gets updated.

Not everyone's aware you can extend interfaces this way so I thought I'd mention it.

All the best.

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