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

Single-member read methods should always return undefined for empty context #2319

Closed
gibson042 opened this Issue May 15, 2015 · 21 comments

Comments

Projects
None yet
5 participants
@gibson042
Member

gibson042 commented May 15, 2015

Derived from gh-2134

Known offenders ($()[ method ]() currently returns null):

  • scrollTop
  • scrollLeft
  • width
  • height
  • innerWidth
  • innerHeight
  • outerWidth
  • outerHeight
@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 15, 2015

Member

Following up on what I was saying in gh-2134, the (outer|inner)width/height ones will be probably be in one PR and the scroll ones in another. I think those can be two tickets. Atomized issues will be more helpful in tracking our progress (and more helpful to me when triaging). Any broad issues that are more guidelines to what specific actions need to be taken can be moved to the Roadmap. Make sense?

Member

timmywil commented May 15, 2015

Following up on what I was saying in gh-2134, the (outer|inner)width/height ones will be probably be in one PR and the scroll ones in another. I think those can be two tickets. Atomized issues will be more helpful in tracking our progress (and more helpful to me when triaging). Any broad issues that are more guidelines to what specific actions need to be taken can be moved to the Roadmap. Make sense?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 15, 2015

Member

I think that runs the risk of incompleteness. I know that these eight methods need to change, but there might be more. Without a tracking ticket, we could land a fix for the scroll methods and a fix for the dimension methods, then erroneously consider the work done. And I really don't want to look in multiple places to understand what's going on.

Member

gibson042 commented May 15, 2015

I think that runs the risk of incompleteness. I know that these eight methods need to change, but there might be more. Without a tracking ticket, we could land a fix for the scroll methods and a fix for the dimension methods, then erroneously consider the work done. And I really don't want to look in multiple places to understand what's going on.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 15, 2015

Member

There's only one place: the Roadmap. You're just tracking it in that instead of a ticket. The item on the Roadmap does not need to be removed just because tickets were closed. We can leave it there until we're sure the it's all done. That's its purpose. Another advantage to this is that we will no longer leave tickets lying open just because we're unsure if it's all done. No more ambiguity in issues. And combined with the Roadmap, there is no incompleteness.

Member

timmywil commented May 15, 2015

There's only one place: the Roadmap. You're just tracking it in that instead of a ticket. The item on the Roadmap does not need to be removed just because tickets were closed. We can leave it there until we're sure the it's all done. That's its purpose. Another advantage to this is that we will no longer leave tickets lying open just because we're unsure if it's all done. No more ambiguity in issues. And combined with the Roadmap, there is no incompleteness.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 15, 2015

Member

Also, having a Roadmap and keeping it updated with the broader issues gives us all one place to see what's going on without sifting through the issues. I know I've found myself many times looking for that one ticket that explains the other ones. I'm proposing we put all that in one place.

Member

timmywil commented May 15, 2015

Also, having a Roadmap and keeping it updated with the broader issues gives us all one place to see what's going on without sifting through the issues. I know I've found myself many times looking for that one ticket that explains the other ones. I'm proposing we put all that in one place.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 16, 2015

Member

The roadmap plus GitHub issues makes two places. And there's no mention of the former in this repository, which makes the project even less accessible to would-be contributors. That's correctable, though, and I'm willing to go with it, but honestly don't see the advantage over adding a new issue tag.

Member

gibson042 commented May 16, 2015

The roadmap plus GitHub issues makes two places. And there's no mention of the former in this repository, which makes the project even less accessible to would-be contributors. That's correctable, though, and I'm willing to go with it, but honestly don't see the advantage over adding a new issue tag.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 16, 2015

Member

I didn't participate in the related ticket, so sorry for this question - why returned value should be undefined?

Sometimes it is known for getters to return null if there is nothing to return. I believe this is why it was done in jQuery in the first place and it is better corresponds with DOM methods, like document.createElement("div").getAttribute("non-existent") // null and it is common in many languages, although such methods usually deal with objects.

On the other hand Object.getOwnPropertyDescriptor({}, "foo"); // undefined.

undefined is easy to do, especially since return value already could be undefined - $({}).scrollTop() // undefined and correlates with ECMA spec, but again, jQuery is DOM-centric so it might make sense to be more align with it?

In any case, i would like for us to documentally establish this, so in the couple of years we wouldn't face the same but the opposite ticket that would propose to always return null instead of undefined.

@BrendanEich, @rwaldron please help us out.

Member

markelog commented May 16, 2015

I didn't participate in the related ticket, so sorry for this question - why returned value should be undefined?

Sometimes it is known for getters to return null if there is nothing to return. I believe this is why it was done in jQuery in the first place and it is better corresponds with DOM methods, like document.createElement("div").getAttribute("non-existent") // null and it is common in many languages, although such methods usually deal with objects.

On the other hand Object.getOwnPropertyDescriptor({}, "foo"); // undefined.

undefined is easy to do, especially since return value already could be undefined - $({}).scrollTop() // undefined and correlates with ECMA spec, but again, jQuery is DOM-centric so it might make sense to be more align with it?

In any case, i would like for us to documentally establish this, so in the couple of years we wouldn't face the same but the opposite ticket that would propose to always return null instead of undefined.

@BrendanEich, @rwaldron please help us out.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 16, 2015

Member

@markelog The DOM arguments are essentially those from #2118, but don't apply here. This ticket is about calling methods on empty collections (e.g., jQuery().height()).

Member

gibson042 commented May 16, 2015

@markelog The DOM arguments are essentially those from #2118, but don't apply here. This ticket is about calling methods on empty collections (e.g., jQuery().height()).

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 16, 2015

Member

I see no difference, if there is nothing to return what should be returned?

Member

markelog commented May 16, 2015

I see no difference, if there is nothing to return what should be returned?

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 16, 2015

Member

The difference is that context collections of any length are valid input, and therefore have defined output. Invalid context, such as your $({}).scrollTop() example, have undefined output and might even throw exceptions.

Member

gibson042 commented May 16, 2015

The difference is that context collections of any length are valid input, and therefore have defined output. Invalid context, such as your $({}).scrollTop() example, have undefined output and might even throw exceptions.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 16, 2015

Member

If these are design documents I think I'd prefer to see them in the wiki, like Adding new features. I could imagine having a document that described our ideals for how jQuery API calls should behave, including a definition of what should happen with an empty context set. Perhaps that could also document the APIs that we know are not consistent with the philosophy and the reasons why (back-compat with old jQuery versions, conformance with W3C output, etc.)

Github Issues isn't very good about generating a Roadmap for us the way Trac did. Or maybe I'm missing a "open issues by milestone" report or even a "sort by milestone"? I suppose Future is the best it can do. Obviously we can be more fine-grained with future milestones if we want.

Back to the subject of this ticket, does it seem like the benefit of consistency will outweigh the work we have to do and potential for compat breakage? Seems like each one of these has to be weighed separately, given a separate ticket, and ultimately thrown to the vagaries of the jQuery code and plugin ecosystem to see how badly we break the web.

Member

dmethvin commented May 16, 2015

If these are design documents I think I'd prefer to see them in the wiki, like Adding new features. I could imagine having a document that described our ideals for how jQuery API calls should behave, including a definition of what should happen with an empty context set. Perhaps that could also document the APIs that we know are not consistent with the philosophy and the reasons why (back-compat with old jQuery versions, conformance with W3C output, etc.)

Github Issues isn't very good about generating a Roadmap for us the way Trac did. Or maybe I'm missing a "open issues by milestone" report or even a "sort by milestone"? I suppose Future is the best it can do. Obviously we can be more fine-grained with future milestones if we want.

Back to the subject of this ticket, does it seem like the benefit of consistency will outweigh the work we have to do and potential for compat breakage? Seems like each one of these has to be weighed separately, given a separate ticket, and ultimately thrown to the vagaries of the jQuery code and plugin ecosystem to see how badly we break the web.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 16, 2015

Member

Right, I'd really rather not use issues anymore for this. Let's use the Roadmap for a while and please atomize issues.

Member

timmywil commented May 16, 2015

Right, I'd really rather not use issues anymore for this. Let's use the Roadmap for a while and please atomize issues.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 16, 2015

Member

By the way, I'll add a note about the Roadmap and what it represents in CONTRIBUTING.md once I've gone through all of our issues and updated it.

Member

timmywil commented May 16, 2015

By the way, I'll add a note about the Roadmap and what it represents in CONTRIBUTING.md once I've gone through all of our issues and updated it.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 18, 2015

Member

The difference is that context collections of any length are valid input

So, what should function return if there is nothing to return?

The difference is that context collections of any length are valid input, and therefore have defined output. Invalid context, such as your $({}).scrollTop() example, have undefined output and might even throw exceptions.

Plain objects are supported, so i'm not sure if this is undefined output. This is the call that doesn't makes sense, same as $("<div/>").scrollTop() but both inputs are supported.

I could imagine having a document that described our ideals for how jQuery API calls should behave, including a definition of what should happen with an empty context set

Would be awesome to have this!

Member

markelog commented May 18, 2015

The difference is that context collections of any length are valid input

So, what should function return if there is nothing to return?

The difference is that context collections of any length are valid input, and therefore have defined output. Invalid context, such as your $({}).scrollTop() example, have undefined output and might even throw exceptions.

Plain objects are supported, so i'm not sure if this is undefined output. This is the call that doesn't makes sense, same as $("<div/>").scrollTop() but both inputs are supported.

I could imagine having a document that described our ideals for how jQuery API calls should behave, including a definition of what should happen with an empty context set

Would be awesome to have this!

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 18, 2015

Member

So, what should function return if there is nothing to return?

"Nothing to return" is call-dependent (e.g., jQuery( "body" ).val() currently returns "" while .prop( "nonExistent" ) returns undefined, and we recently tried to change .attr( "non-existent" ) from undefined to null). But return values for invalid input are unspecified, and I'm trying to establish consistency around jQuery()[ getter ]() returning undefined.

Plain objects are supported, so i'm not sure if this is undefined output. This is the call that doesn't makes sense, same as $("<div/>").scrollTop() but both inputs are supported.

Plain objects are not supported: "At present, the only operations supported on plain JavaScript objects wrapped in jQuery are: .data(), .prop(), .on(), .off(), .trigger() and .triggerHandler()." See also jquery/api.jquery.com/issues/709 .

Member

gibson042 commented May 18, 2015

So, what should function return if there is nothing to return?

"Nothing to return" is call-dependent (e.g., jQuery( "body" ).val() currently returns "" while .prop( "nonExistent" ) returns undefined, and we recently tried to change .attr( "non-existent" ) from undefined to null). But return values for invalid input are unspecified, and I'm trying to establish consistency around jQuery()[ getter ]() returning undefined.

Plain objects are supported, so i'm not sure if this is undefined output. This is the call that doesn't makes sense, same as $("<div/>").scrollTop() but both inputs are supported.

Plain objects are not supported: "At present, the only operations supported on plain JavaScript objects wrapped in jQuery are: .data(), .prop(), .on(), .off(), .trigger() and .triggerHandler()." See also jquery/api.jquery.com/issues/709 .

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 18, 2015

Member

Plain objects are not supported: "At present, the only operations supported on plain JavaScript objects wrapped in jQuery are: .data(), .prop(), .on(), .off(), .trigger() and .triggerHandler()." See also jquery/api.jquery.com#709 .

Hm, missed that link, okay then

"Nothing to return" is call-dependent (e.g., jQuery( "body" ).val() currently returns "" while .prop( "nonExistent" ) returns undefined, and we recently tried to change .attr( "non-existent" ) from undefined to null).

I'm not asking what it returns now, i'm asking what should it return.

But return values for invalid input are unspecified, and I'm trying to establish consistency around jQuery() getter returning undefined

Whatever those getters would return, i think we should follow some general idea, if it should be undefined then we should explain why undefined specifically, why not null and why not empty string, etc.

Right now, it seems we trading one value for another without any reason at all.

Member

markelog commented May 18, 2015

Plain objects are not supported: "At present, the only operations supported on plain JavaScript objects wrapped in jQuery are: .data(), .prop(), .on(), .off(), .trigger() and .triggerHandler()." See also jquery/api.jquery.com#709 .

Hm, missed that link, okay then

"Nothing to return" is call-dependent (e.g., jQuery( "body" ).val() currently returns "" while .prop( "nonExistent" ) returns undefined, and we recently tried to change .attr( "non-existent" ) from undefined to null).

I'm not asking what it returns now, i'm asking what should it return.

But return values for invalid input are unspecified, and I'm trying to establish consistency around jQuery() getter returning undefined

Whatever those getters would return, i think we should follow some general idea, if it should be undefined then we should explain why undefined specifically, why not null and why not empty string, etc.

Right now, it seems we trading one value for another without any reason at all.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 May 18, 2015

Member

I'm not asking what it returns now, i'm asking what should it return.

It's still call-dependent. I maintain that $el.attr( "nonexistent" ) should return null (like DOM getAttribute) and $el.prop( "nonexistent" ) should return undefined (like JavaScript property accessors), even though backwards compatibility makes such a change difficult.

Right now, it seems we trading one value for another without any reason at all.

Most methods return undefined already—.css, .data, .prop, .attr, .offset, etc. We're not trading values, we're bringing the exceptions in line.

Member

gibson042 commented May 18, 2015

I'm not asking what it returns now, i'm asking what should it return.

It's still call-dependent. I maintain that $el.attr( "nonexistent" ) should return null (like DOM getAttribute) and $el.prop( "nonexistent" ) should return undefined (like JavaScript property accessors), even though backwards compatibility makes such a change difficult.

Right now, it seems we trading one value for another without any reason at all.

Most methods return undefined already—.css, .data, .prop, .attr, .offset, etc. We're not trading values, we're bringing the exceptions in line.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 18, 2015

Member

We're not trading values

Okay, we changing return values, if that phrasing is more suiting. I think we need verbose explanation of why one value but not the other, then follow that general principal and recommended to the plugins to do the same.

Member

markelog commented May 18, 2015

We're not trading values

Okay, we changing return values, if that phrasing is more suiting. I think we need verbose explanation of why one value but not the other, then follow that general principal and recommended to the plugins to do the same.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 7, 2015

Member

I updated the wiki page to reflect this as an API design goal. Is everyone okay with doing this? If so I can take it with a 3.0 milestone.

Member

dmethvin commented Nov 7, 2015

I updated the wiki page to reflect this as an API design goal. Is everyone okay with doing this? If so I can take it with a 3.0 milestone.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Nov 8, 2015

Member

👍 from me.

Member

gibson042 commented Nov 8, 2015

👍 from me.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Nov 9, 2015

Member

@dmethvin 👍. BTW, can we change "tenets" to "rules"? The former word may be a barrier to non-native speakers, it's not commonly used.

Member

mgol commented Nov 9, 2015

@dmethvin 👍. BTW, can we change "tenets" to "rules"? The former word may be a barrier to non-native speakers, it's not commonly used.

@dmethvin dmethvin added this to the 3.0.0 milestone Nov 9, 2015

@dmethvin dmethvin self-assigned this Nov 9, 2015

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 9, 2015

Member

Just a note, this is a second reversal from trac-12283 but I still think it is the correct return for an empty set and in any case acceptable for a 3.0 milestone that allows breaking changes.

Member

dmethvin commented Nov 9, 2015

Just a note, this is a second reversal from trac-12283 but I still think it is the correct return for an empty set and in any case acceptable for a 3.0 milestone that allows breaking changes.

dmethvin added a commit to dmethvin/jquery that referenced this issue Nov 9, 2015

dmethvin added a commit to dmethvin/jquery that referenced this issue Nov 9, 2015

dmethvin added a commit that referenced this issue Nov 10, 2015

@dmethvin dmethvin closed this in 2937019 Nov 10, 2015

gibson042 added a commit to gibson042/api.jquery.com that referenced this issue Mar 3, 2016

AurelioDeRosa added a commit to jquery/api.jquery.com that referenced this issue Mar 4, 2016

AurelioDeRosa added a commit to jquery/api.jquery.com that referenced this issue Mar 4, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

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