Return null from .attr when attributes don't exist #2118

Closed
gibson042 opened this Issue Mar 5, 2015 · 8 comments

Comments

Projects
None yet
6 participants
@gibson042
Member

gibson042 commented Mar 5, 2015

.attr("nonexistent") currently returns undefined, probably from the days before .prop. But the native DOM getAttribute and Sizzle.attr both return null in such cases. Also, .attr( name, null ) removes attributes, and it would be very convenient to allow universal round-tripping.

Let's consider changing the return value to accommodate.

Note that setter treatment of undefined is explicitly out-of-scope for this ticket, as are .prop and .data round-tripping, although those are certainly related topics.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 9, 2015

Member

@dmethvin We're leaning towards doing this for beta. Do you know of a use case that would make this catastrophic?

Member

timmywil commented Mar 9, 2015

@dmethvin We're leaning towards doing this for beta. Do you know of a use case that would make this catastrophic?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Mar 9, 2015

Member

It seems like returning null is a good idea, although who knows what it will break in the wild. The only way to find out is to change it! We should continue to return undefined for empty jQuery collections tho.

Member

dmethvin commented Mar 9, 2015

It seems like returning null is a good idea, although who knows what it will break in the wild. The only way to find out is to change it! We should continue to return undefined for empty jQuery collections tho.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 9, 2015

Member

We should continue to return undefined for empty jQuery collections tho.

Thanks; that reminded me to create #2134.

Member

gibson042 commented Mar 9, 2015

We should continue to return undefined for empty jQuery collections tho.

Thanks; that reminded me to create #2134.

@timmywil timmywil closed this in aaeed53 Mar 16, 2015

timmywil added a commit that referenced this issue Mar 16, 2015

Attributes: return null when attribute does not exist
Fixes gh-2118
Close gh-2129

Conflicts:
	test/unit/attributes.js
@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Mar 23, 2015

Member

It seems like returning null is a good idea, although who knows what it will break in the wild.

Of course the answer turns out to be jQuery UI :-)

I'm working on updates right now.

Member

scottgonzalez commented Mar 23, 2015

It seems like returning null is a good idea, although who knows what it will break in the wild.

Of course the answer turns out to be jQuery UI :-)

I'm working on updates right now.

@scottgonzalez scottgonzalez referenced this issue in jquery/jquery-ui Mar 25, 2015

Closed

Null attributes #1516

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 30, 2015

Member

The team decided to revert this due to breakages in UI and mobile.

Member

timmywil commented Mar 30, 2015

The team decided to revert this due to breakages in UI and mobile.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Mar 15, 2016

Member

We backed this out for 1.12/2.2 which definitely seems right, but it's also excluded from 3.0. Should it be? That is, do we want a non-existent attribute to return null as proposed here? If not I want to remove the milestone and tag.

Member

dmethvin commented Mar 15, 2016

We backed this out for 1.12/2.2 which definitely seems right, but it's also excluded from 3.0. Should it be? That is, do we want a non-existent attribute to return null as proposed here? If not I want to remove the milestone and tag.

@mgol mgol removed this from the 3.0.0 milestone Mar 23, 2016

@mgol mgol added the Needs review label Mar 23, 2016

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 23, 2016

Member

I removed the milestone due to the revert and opened the issue & added the "Needs review" label to account for the Dave's question.

Member

mgol commented Mar 23, 2016

I removed the milestone due to the revert and opened the issue & added the "Needs review" label to account for the Dave's question.

@mgol mgol reopened this Mar 23, 2016

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Mar 28, 2016

Member

We backed this out for 1.12/2.2 which definitely seems right, but it's also excluded from 3.0. Should it be?

I remember this discussion, we reverted cause it might be dangerous to do which proved the UI breakage, where is literally no gain from, except to align with standard, whereas behaviour in DOM spec is very questionable in the first place, since in all others JS API you get undefined value not null value

Member

markelog commented Mar 28, 2016

We backed this out for 1.12/2.2 which definitely seems right, but it's also excluded from 3.0. Should it be?

I remember this discussion, we reverted cause it might be dangerous to do which proved the UI breakage, where is literally no gain from, except to align with standard, whereas behaviour in DOM spec is very questionable in the first place, since in all others JS API you get undefined value not null value

@markelog markelog closed this Mar 28, 2016

@markelog markelog removed the Needs review label Mar 28, 2016

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery locked as resolved and limited conversation to collaborators Jun 18, 2018

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