Core: Return empty array instead of null for parseHTML("") #1998
Conversation
| @@ -15,7 +15,7 @@ define([ | |||
| // defaults to document | |||
| // keepScripts (optional): If true, will include scripts passed in the html string | |||
| jQuery.parseHTML = function( data, context, keepScripts ) { | |||
| if ( !data || typeof data !== "string" ) { | |||
| if ( typeof data !== "string" ) { | |||
| return null; | |||
| } | |||
There was a problem hiding this comment.
I'd even support a followup to drop this if entirely. If non-string input really causes a problem, it should be visible as an exception.
There was a problem hiding this comment.
I wouldn't support that. parseHTML via createHTMLDocument does not throw an error with parseHTML(1) or parseHTML({}).
There was a problem hiding this comment.
However, maybe we could just return an empty array here.
There was a problem hiding this comment.
The docs say the first arg is a String and that it returns Array. I don't think we should ever be returning null, despite the unit test. 😄 If the first arg is not a string it seems either all bets are off or we should turn it into a string for cases we feel are possible (numbers, null, undefined)? I would be fine with removing this entirely but have a feeling it would be like our jQuery.parseJSON(null) fiasco.
There was a problem hiding this comment.
I don't think we should ever be returning null, despite the unit test.
This is the intent of my suggestion. Just dropping the if and letting values propagate through gives good (and mostly expected) array results (http://jsfiddle.net/evse952q/), so we need not check for any special input or construct any special output (and it's worth noting that our internal uses always provide string input as documented).
Edit: And whatever weirdness exists in the output stems from buildFragment, which could perhaps also use a similar treatment at
Line 203 in a3779bc
elem != null instead of elem || elem === 0).
There was a problem hiding this comment.
The results for numbers and true are inconsistent. I'd rather they return empty array.
There was a problem hiding this comment.
I dunno @timmywil , looking through the whole execution path (which I should have done originally) it seems like dropping the check only affects really strange inputs and not in a catastrophic way.
There was a problem hiding this comment.
Yea, it's not terrible. i just think if we're going to change it, why not match native behavior?
There was a problem hiding this comment.
Native behavior always casts to string except null (which is inconsistent between browsers): http://jsfiddle.net/evse952q/1/
Achieving that in jQuery would mean adopting my above buildFragment suggestion and casting (some) nullish values to string in parseHTML.
|
Are you willing to update or re-PR for the more comprehensive changes, @Krinkle? |
Krinkle
changed the title
|
@gibson042 Done. |
Fixes #1997