[browserified] Fails on IE7 (and IE6)

[ixti]

While support of IE6 is close to necrophilia, IE7 is still one of the things that make our days not so shiny. By some reason js-yaml ignores new-line breaks on IE7.

This works:

{ foo: bar, baz: 123 }

And this one doesn't:

foo: bar
baz: 123

To demonstrate problem closer:

- foo
- bar

Becomes:

[ 'foo - baz' ]

Also, integers are becomes integers ONLY with strict type casting:

{ foo: !!int 123, bar: 123 }

The above will become:

{ foo: 123, bar: '123' }

[ixti]

If this issue is a must-have for you, feel free to submit a patch, or at least thumb up this issue.

[dccourt]

I took a look at this, because my life is sad enough to involve IE7.

Looks like it's as simple as IE7 not supporting string subscripting. This patch seems to fix it:

--- js/js-yaml.js       (revision 50331)
+++ js/js-yaml.js       (working copy)
@@ -934,7 +934,9 @@
   }

   while (length) {
-    ch = this.buffer[this.pointer];
+//  XXX IE7 or less don't support string subscripting like this.
+//    ch = this.buffer[this.pointer];
+    ch = this.buffer.charAt(this.pointer);
     this.pointer += 1;
     this.index += 1;

[ixti]

@Fanjita Thanks for pointing this out. Can you please check new browserified version works now?

[dccourt]

Thanks. I see you included a few cases that I'd missed, but it looks like you missed one:

    4379    4379  |BaseResolver.prototype.resolve = function resolve(kind, value, implicit) {
    4380    4380  |  var resolvers, i, tag, regexp;
    4381    4381  |
    4382    4382  |  if (kind === _nodes.ScalarNode && implicit && implicit[0]) {
    4383    4383  |    if (value === '') {
    4384    4384  |      resolvers = this.yamlImplicitResolvers[''] || [];
    4385    4385  |  } else {
+           4386  |      resolvers = this.yamlImplicitResolvers[value.charAt(0)] || [];
-   4386          |      resolvers = this.yamlImplicitResolvers[value[0]] || [];
    4387    4387  |        }
    4388    4388  |
    4389    4389  |    resolvers = resolvers.concat(this.yamlImplicitResolvers[null] || []);
    4390    4390  |
    4391    4391  |    for (i = 0; i < resolvers.length; i += 1) {
    4392    4392  |      tag = resolvers[i][0];
    4393    4393  |      regexp = resolvers[i][1];
    4394    4394  |

Without this, the parser isn't able to recognise any of the primitive types - e.g. bools become strings.

Otherwise, it seems to be working well on IE7 and IE8.

Note that as well as the es5-shims, I also needed to add this shim manually to make things work properly:

if (typeof Object.getOwnPropertyNames !== "function") {
    Object.getOwnPropertyNames = function (obj) {
        var keys = [];

        // Only iterate the keys if we were given an object, and
        // a special check for null, as typeof null == "object"
        if (typeof obj === "object" && obj !== null) {    
            // Use a standard for in loop
            for (var x in obj) {
                // A for in will iterate over members on the prototype
                // chain as well, but Object.getOwnPropertyNames returns
                // only those directly on the object, so use hasOwnProperty.
                if (obj.hasOwnProperty(x)) {
                    keys.push(x);
                }
            }
        }

        return keys;
    }
}

[ixti]

Thanks once again. Hope now it will work :D
About getOwnPropertyNames. In fact no need - shims were simply splitted into shims and shams. I have updated README explicitly saying that shams are also required. Also demo was updated with these shams, so you can easily check if it solves problems or not on js-yaml demo.

Please let me know if this is not a bug in IE7 anymore, so I'll be happy to close this issue :D

[dccourt]

The demo seems broken on IE7 - I just get the error "The value of the property 'owns' is null or undefined, not a Function object". That seems to be a problem with the getOwnPropertyDescriptor sham, which is the only piece of code in the demo that contains "owns".

I'm wary of using the shams - they're separated out because they're incomplete or broken. Replacing my existing custom shim for getOwnPropertyNames with the version from es5-shams does seem to work for me, though, on IE7 and above - maybe the best fix is to replace uses of getOwnPropertyNames in js-yaml with "Object.keys(obj)", which is all that the sham does, and then you shouldn't need to use the shams at all.

[ixti]

Hm. I tend to agree. I've updated browserified version and demo with your proposal.

[dccourt]

Bah. The latest browserified script works fine for everything I need to do with it on IE7 and IE8.

But the demo still fails. I added a shim for Object.create (taking the version from es5-shams), which got a bit further, but then it complains about getOwnPropertyDescriptor. I can't find a shim for that that doesn't use owns(), which also seems to be missing from IE7/8, and is brutal to google for.

I'll try to find time to look at whether the use of getOwnPropertyDescriptor is necessary or not.

[dccourt]

This patch to the demo fixes things for IE7 (it turns out that getOwnPropertyDescriptor is only being used for displaying the results):

   utils.js.original (03 Dec 2012 15:59:48)
           utils.js (04 Dec 2012 11:21:00)
===================
     172     172  |  function formatProperty(ctx, value, recurseTimes, visibleKeys, key, array) {
     173     173  |    var name, str, desc;
+            174  |    desc = (Object.getOwnPropertyDescriptor && Object.getOwnPropertyDescriptor(value, key)) || { value: value[key] };
-    174          |    desc = Object.getOwnPropertyDescriptor(value, key) || { value: value[key] };
     175     175  |    if (desc.get) {
     176     176  |      if (desc.set) {
===================

IE8 is still broken, though, with a very unhelpful error of "Object doesn't support this action". I'm still looking into that.

[dccourt]

Seems that IE8's Object.getOwnPropertyDescriptor() method is broken for non-DOM objects (http://msdn.microsoft.com/en-us/library/ie/dd548686%28v=vs.94%29.aspx).

This works as a dirty hack to make the demo functional:

   utils.js.original (03 Dec 2012 15:59:48)
           utils.js (04 Dec 2012 12:19:40)
===================
     172     172  |  function formatProperty(ctx, value, recurseTimes, visibleKeys, key, array) {
     173     173  |    var name, str, desc;
+            174  |    desc = (
+            175  |      //Object.getOwnPropertyDescriptor && Object.getOwnPropertyDescriptor(value, key)
+            176  |      0
+            177  |      ) || { value: value[key] };
-    174          |    desc = Object.getOwnPropertyDescriptor(value, key) || { value: value[key] };
     175     178  |    if (desc.get) {
     176     179  |      if (desc.set) {
===================

I can't see any obvious way to autodetect that the method is broken for IE8, nor a working shim/sham implementation. Possibly you could simply undefine Object.getOwnPropertyDescriptor, and use the es5-sham implementation for all cases, which is probably good enough for the demo.

Alternatively I guess you can autodetect IE7 or IE8, and use something like the hack above.

I'm not really sure which is least ugly!

[puzrin]

@ixti Looks like desc not needed at all in our case. inspect code was copied from node.js internals many time ago. It's time to rework it now:

• copy fresh code (it' was improved, AFAIK)
• rip out unnecesary parts, that can cause prowser issues

[dervus]

Version 2.x.x reads input using String#charCodeAt and there is no getOwnPropertyNames in the code. So, is this issue still actual?

[ixti]

Closing an issue as code was completely rewritten and it needs to be tested in IE completely once again.