@campersau Good catch.

1. In prop (https://github.com/jquery/jquery/blob/master/src/attributes/prop.js#L32) it does happen. Should that be removed too?
2. Regarding the comment in Attr: remove unnecessary element null check #2201 - I think !elem should be then checked before we access the nodeType property. (as per https://github.com/jquery/jquery/blob/master/src/attributes/attr.js#L27)

Thoughts?

This is good (-35 min+gz). Can you also look at increasing the return similarities (from value !== undefined onward)?

@gibson042 No problem.

Regarding checking if elem exists (in either prop or attr). What are the thoughts?

Regarding checking if elem exists (in either prop or attr). What are the thoughts?

Drop it from both.

Ok rebased what was noted so far... Will continue trying to create similarities...

@gibson042 I think the module selector-sizzle is missing in prop.js (but found in attr.js )- we use jQuery.isXMLDoc (https://github.com/jquery/jquery/blob/master/src/attributes/prop.js#L36)

There is also an inconsistency where in prop( someKey, null ) - the null will get assigned and in attr( someKey, null ) is translated to jQuery.removeAttr.

Is there are reason for that?

edit I now see that in attr the value gets coerced into a string. I don't think that the remove behavior is documented though. Either way I think the behavior should be consistent between them.

Relevant code: https://github.com/jquery/jquery/blob/master/src/attributes/attr.js#L49-L50

I think the module selector-sizzle is missing in prop.js (but found in attr.js )- we use jQuery.isXMLDoc (https://github.com/jquery/jquery/blob/master/src/attributes/prop.js#L36)

What do you mean? That method is provided by selector, which is part of the minimum build set.

I mean "../selector" is imported in attr.js but not in prop.js even though they both use the method isXMLDoc found in selector-sizzle.

There is also an inconsistency where in prop( someKey, null ) - the null will get assigned and in attr( someKey, null ) is translated to jQuery.removeAttr.

Is there are reason for that?

It is intentional, but surprisingly not documented! That inconsistency should be preserved by this pull request, as should the string coercion in attr. DOM attributes are an inherently less powerful data store than Javascript properties.

Ah. prop.js should also import ../selector.

@gibson042 Great... I think I implemented all of the changes. Also flattened the multiple nested ifs (I highly dislike the nested ifs and prefer the early returns.

@pgilad we're ready to land this, but could you also provide a pull for the compat branch? Otherwise it would be hard for us to port these changes in there

@markelog Done - #2426
Also rebased this PR against master

Thank you!