Skip to content

Core: Return empty array instead of null for parseHTML("") #1998

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.

Sign up for GitHub

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

Closed
Krinkle wants to merge 1 commit into from
Closed

Core: Return empty array instead of null for parseHTML("") #1998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/parseHTML.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ 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" ) {
return null;
if ( typeof data !== "string" ) {
return [];
}
Copy link
Member

@gibson042 gibson042 Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@timmywil timmywil Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't support that. parseHTML via createHTMLDocument does not throw an error with parseHTML(1) or parseHTML({}).

Copy link
Member

@timmywil timmywil Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, maybe we could just return an empty array here.

Copy link
Member

@dmethvin dmethvin Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gibson042 gibson042 Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jquery/src/manipulation.js

Line 203 in a3779bc

if ( elem || elem === 0 ) {
(e.g., check for elem != null instead of elem || elem === 0).

Copy link
Member

@timmywil timmywil Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The results for numbers and true are inconsistent. I'd rather they return empty array.

Copy link
Member

@dmethvin dmethvin Jan 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@timmywil timmywil Jan 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's not terrible. i just think if we're going to change it, why not match native behavior?

Copy link
Member

@gibson042 gibson042 Jan 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( typeof context === "boolean" ) {
keepScripts = context;
Expand Down
13 changes: 9 additions & 4 deletions test/unit/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,13 +1301,18 @@ test("jQuery.proxy", function(){
});

test("jQuery.parseHTML", function() {
expect( 18 );
expect( 23 );

var html, nodes;

equal( jQuery.parseHTML(), null, "Nothing in, null out." );
equal( jQuery.parseHTML( null ), null, "Null in, null out." );
equal( jQuery.parseHTML( "" ), null, "Empty string in, null out." );
deepEqual( jQuery.parseHTML(), [], "Without arguments" );
deepEqual( jQuery.parseHTML( undefined ), [], "Undefined" );
deepEqual( jQuery.parseHTML( null ), [], "Null" );
deepEqual( jQuery.parseHTML( false ), [], "Boolean false" );
deepEqual( jQuery.parseHTML( 0 ), [], "Zero" );
deepEqual( jQuery.parseHTML( true ), [], "Boolean true" );
deepEqual( jQuery.parseHTML( 42 ), [], "Positive number" );
deepEqual( jQuery.parseHTML( "" ), [], "Empty string" );
throws(function() {
jQuery.parseHTML( "<div></div>", document.getElementById("form") );
}, "Passing an element as the context raises an exception (context should be a document)");
Expand Down