Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#6903 closed defect (fixed)

dojo.toJson does not work when dom nodes are included

Reported by: guest Owned by: Kris Zyp
Priority: high Milestone: 1.2
Component: General Version: 1.1.1
Keywords: Cc:
Blocked By: Blocking:

Description

If the object includes a dom node, dojo.toJson does not work properly. For example:

  {
     dom: someDomNode,
     test:"dojo"
  }

leads to

{dom:,"test":"dojo"}

Reason is the following code in json.js (line 60)

if(it.nodeType && it.cloneNode){ // isNode
  return ""; 
}

Perhaps this should be something like:

if(it.nodeType && it.cloneNode){ // isNode
  return "\"\""; // return "null"; 
}

It would be even better if the dom node would not show up in the resulting json string ....

Attachments (1)

json.2.diff (459 bytes) - added by kriszyp 11 years ago.
Removal of DOM check

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by Adam Peller

Owner: changed from anonymous to kriszyp

comment:2 Changed 11 years ago by Adam Peller

see #4186

comment:3 Changed 11 years ago by kriszyp

Attached a patch for review

comment:4 Changed 11 years ago by Adam Peller

dunno. I'm on the fence, maybe leaning towards outerHTML just so we don't have to worry about exceptions. What does Crockford do?

comment:5 Changed 11 years ago by kriszyp

Crockford's library does not have any special processing for DOM nodes; the DOM nodes are processed as objects, and their properties are serialized like a normal JS object. We could also handle it that way, that would be the most compact solution. outerHTML is fine with me as well, my biggest objection to that is the increase in code side. X-browser outerHTML is probably about 4-5 lines of code.

comment:6 Changed 11 years ago by Adam Peller

I'm +1 on the minimalist approach, especially if it's consistent with Crockford's library.

Changed 11 years ago by kriszyp

Attachment: json.2.diff added

Removal of DOM check

comment:7 Changed 11 years ago by kriszyp

This patch look allright?

comment:8 Changed 11 years ago by kriszyp

DOM nodes have cycles, so this technique (no DOM check) causes maximum recursion exceptions. I am fine with that, but I don't know if that would be popular (probably result in another bug report).

comment:9 Changed 11 years ago by James Burke

It seems that the maximum recursion error will be guaranteed to occur if there is a DOM node? If so, then it seems no better than the current behavior, and at least the current behavior would fail faster, maybe give a better error indication (easier to inspect the malformed JSON? I'm not sure, have not tried it).

So I vote for either returning something like null or empty string, or just throwing an exception if there is a DOM node, since node-to-JSON has not been worked out formally. It would be curious to note the behavior of other browser objects, like window, document, location, an XHR object behave. Perhaps match that behavior for what we do with DOM nodes?

If that does not help define the behavior, maybe just return "[DOMNode]"?

comment:10 Changed 11 years ago by kriszyp

Malformed JSON does not fail faster, rather it fails later, because it fails when the recipient tries to parse it, versus failing during serialization which would certainly be greatly preferable to the current behavior. Creating malformed JSON is simply unacceptable as far as I am concerned, I would rather see a recurse exception.

However, I initially proposed throwing an exception when encountering a DOM node, this fails fastest and provides a more informative error (rather than waiting for recurse error, which is rather cryptic). The drawback to this is the extra 2-3 lines of code. Returning null (or empty string) would be roughly the same code, and I don't care either way. More of question of it is worth it...

I am sure window and document would behave the same way as a DOM node (it has cycles). I don't think the XHR and location objects have cycles and I think they would actually be serialized as an object with properties.

comment:11 Changed 11 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

(In [14078]) Throw an exception when a DOM node is encountered in JSON serialization (fixes #6903)

comment:12 Changed 11 years ago by Kris Zyp

(In [14079]) Throw an exception when a DOM node is encountered in JSON serialization (fixes #6903)

comment:13 Changed 11 years ago by bill

Milestone: 1.2

marking tickets closed in the last three months w/blank milestone to milestone 1.2.

comment:14 Changed 9 years ago by bill

Owner: changed from kriszyp to Kris Zyp
Note: See TracTickets for help on using tickets.