baconjs / bacon.js

Functional reactive programming library for TypeScript and JavaScript
https://baconjs.github.io
MIT License
6.47k stars 330 forks source link

Bacon._.toString throws exception for hidden inputs #315

Closed also closed 10 years ago

also commented 10 years ago

In Chrome and Firefox, simply accessing the selectionStart property of a hidden input element throws an exception: https://bugzilla.mozilla.org/show_bug.cgi?id=746906

The property is enumerable, so this line tries to access it: https://github.com/baconjs/bacon.js/blob/0.7.0/src/Bacon.coffee#L1409

This means if an object in your stream has a hidden input, and you do stream.log(), nothing is logged.

<input type="hidden" id="input"/>
console.log Bacon._.toString input

(fiddle)

raimohanska commented 10 years ago

Wow. Didn't see that coming :(

Any suggestions for fix? Try-catch around all the risky line?

rpominov commented 10 years ago

May be adding a hasOwnProperty check would be enough?

By the way, CoffeeScript has nice syntax to do that:

for own key, value of object
raimohanska commented 10 years ago

@pozadi sounds good!

rpominov commented 10 years ago

No, just checked it. input.hasOwnProperty('selectionStart') returns true.

raimohanska commented 10 years ago

A try-catch might be in order then? Although I don't like the idea of patching webkit's (or whatnot's) bugs in Bacon.js.

also commented 10 years ago

According to the HTML5 spec, the exception is unfortunately the correct behavior: http://www.w3.org/html/wg/drafts/html/master/forms.html#textFieldSelection

The line that throws the exception is in generated JavaScript:

          return "{" + ((function() {
            var _results;
            _results = [];
            for (key in obj) {
              value = obj[key];
              _results.push(_.toString(key) + ":" + _.toString(value));
            }
            return _results;
          })()) + "}";

I'd suggest desugaring the CoffeeScript loop into something like this so that the object's other properties are still included:

for key of obj
  value = try
    obj[key]
  catch ex
    ex
  _.toString(key) + ':' + _.toString(value)
raimohanska commented 10 years ago

So try-catch it is. How do we simulate the issue in our tests that are run in node.js?

phadej commented 10 years ago
obj = {}

Object.defineProperty obj, "prop",
  enumerable: true
  get: ->
    throw new Error "an error"

for own key, value of obj
  console.log key, value

EDIT: I don't know why someone wants to do anything like that, but it's possible!

raimohanska commented 10 years ago

Thanks guys! I added the try-catch with associated test. Added some extra test coverage too to make sure numbers, bools, dates etc render appropriately.