evanmoran / oj

Unified web templating for the people. Thirsty people.
MIT License
444 stars 7 forks source link

info in oj.copyProperty is undefined #6

Closed Zemke closed 11 years ago

Zemke commented 11 years ago

The info variable in the oj.copyProperty method is occasionally undefined causing the whole app to stop.

I worked around it this way:

oj.copyProperty = function(dest, source, propName) {
        var info;
        info = Object.getOwnPropertyDescriptor(source, propName);
        if (info === undefined) {
            info = {
                value: [],
                enumerable: false,
                writable: false,
                configurable: true
            };
        }

        if (info.value != null) {
            info.value = _clone(info.value);
        }
        return Object.defineProperty(dest, propName, info);
    };

https://github.com/ojjs/oj/blob/master/src/oj.js#L344

evanmoran commented 11 years ago

Thanks for writing this. I honestly didn't think getOwnPropertyDescriptor could fail. Good catch, and I'll be sure to put it in the next release.

evanmoran commented 11 years ago

@Zemke Did you have a repro for this? It appears getOwnPropertyDescriptor doesn't walk the prototype chain.. Is that where you are running into this? Trying to decide if it is worth creating a recursive helper for getOwnPropertyDescriptor and if empty array, [], is the best default.

Zemke commented 11 years ago

I have no public repository for this. I don't know if the issue had anything to do with the prototype chain. If you want I will reproduce this and get you more information.

evanmoran commented 11 years ago

@Zemke If you can repro it that would be greatly appreciated=). I'm mostly trying to write a unit test and wondering where the issue is coming from; copyProperty is used in oj.createType's inheritance mechanism, so maybe there are special cases to inheritance I'm missing. I definitely agree some sort of defaulting is needed here.

Secondly, I'm wondering why you chose value: [].

Zemke commented 11 years ago

I will reproduce it. Expect an answer on the weekend.

I chose value: [] because I had seen it was assigned that every now and then when Object.getOwnPropertyDescriptor wasn't returning undefined. Moreover it just worked.

evanmoran commented 11 years ago

Thanks man. I'll probably submit your fix in 0.2.2 just so no one else runs into this in the mean time. If you repro it I'll add a unit test and maybe adjust the fix depending on what is causing it.

Zemke commented 11 years ago

Hmm... can't figure out the problem. I can't jump into the function. It returns undefined on its fourth call when propName is "after".

evanmoran commented 11 years ago

I added your fix in v0.2.2. Let me know if it works for you!

Zemke commented 11 years ago

Great! I will report back soon.