Open garryyao opened 10 years ago
Hello @garryyao,
It seems we should run a quick test to ensure that Object.defineProperties performs its basic function correctly. :+1:
Should be an easy addition. Thanks for posting this issue.
Have you found any other inconsistencies or problems on Android?
-- John
Hm. I just checked our source code and we already test the basic functionality of the native features.
Here's our test:
try {
// test it
Object.defineProperties(object, { 'sentinel2': { value: 1 } });
return 'sentinel2' in object;
}
catch (ex) { /* squelch */ }
For some reason, your tests are failing, though. Do you have any idea what the substantial difference is? I wonder if it is the fact that you are declaring a function's prototype.
Nm. I see the problem. We should change the code to test that the value is the same, not just that it exists:
try {
// test it
Object.defineProperties(object, { 'sentinel2': { value: true } });
return 'sentinel2' in object && object.sentinel2 === true;
}
catch (ex) { /* squelch */ }
Hey @garryyao, I just pushed a quick fix to the dev branch. I don't have access to all those Android versions, atm. Do you think you could try it? Thanks! -- J
@unscriptable, sorry for late reply.
I am original writer of these test codes. Garry help me to post the issue here.
I think you misunderstood about these codes. The point is that 'prototype' can not be defined as a reserved words on some Android version. Maybe, first step, we should create a new test to check whether hosts support 'prototype' in 'Object.defineProperties'.
Yes, defining the prototype
property is the key for reproducing this issue, which is quite special a property on function.
@KimiZH Can you come with a poly TC that fails you on on those browsers?
Hey guys,
I'm trying to understand what to do next. Should we change the tests so that they try to define a property named "prototype" instead of "sentinel1" or "sentinel2"? Is that enough to detect the problem? I'm not convinced that that is the right solution.
For old Android, it seems like we should allow Object.defineProperty to work as it normally does on all property names except "prototype". Therefore, we need an additional test just for detecting failures when defining "prototype" on functions. Does this sound right?
@jaredcacurak: is this something you might have some time to work on?
-- John
I'd be glad to look into it @unscriptable.
Cool @jaredcacurak! Let's push something to a branch and see if @garryyao or @KimiZH can verify it works.
@garryyao @KimiZH Does the test, added in the commit above, recreate the issue?
I think we can replace the native function if 'prototype' property can not be defined. https://github.com/KimiZH/poly/commit/bc8ef6dc128ac4c6f533a24bd53c6a487b122104
Object.defineProperty has the same issue. https://github.com/KimiZH/poly/commit/c0836631623b1467470dfca4c0047724e38f5d61
This issue looks to be the culprit.
It looks good. I think this would resolve.
The changes are good, while we might want to check the speciality of descriptor of prototype property as well, illustrated by the following tests:
// tc.1 - Object.getOwnPropertyDescriptor
function Foo() {}
var desc = Object.getOwnPropertyDescriptor(Foo, 'prototype');
console.assert(desc.configurable === false, "function's prototype property should NOT be configurable.");
console.assert(desc.enumerable === false, "function's prototype property should NOT be enumerable.");
console.assert(desc.writable === true, "function's prototype property should be writable.");
// tc.2 - Object.defineProperty
function Foo() {}
Object.defineProperty(Foo, 'prototype', {value: 'foo'});
console.assert(new Foo().__proto__ === Object.prototype, "function's prototype property should fallback to Object.prototype when it encounters invalid values, e.g. of non-object type.");
// tc.3 - Object.freeze
var foo = {};
function Bar() {}
Object.defineProperty(Bar, 'prototype', {value: foo});
Object.defineProperty(foo, 'name', {value: 'bar', writable: false});
var bar = new Bar();
bar.name = 'foo';
console.assert(bar.name === 'bar', 'object property should NOT writable specified by the prototype');
@garryyao can you elaborate on your previous comment?
We have to be careful not to test for every bug in every version of every browser. If these bugs re causing problems in your applications, @garryyao, then perhaps we should open another issue? Otherwise, I think we should just let old Android bugs die out with old Android phones. :)
We have to be careful not to test for every bug in every version of every browser.
So poly doesn't end up being 100KB. :)
@jaredcacurak I suggest using native method if property is not 'prototype'. Beacuse other properties (configurable, enumerable, writable) should work. https://github.com/KimiZH/poly/commit/51c2cc70853604f5c2e8e7cee4e10b122f3a3549
Hey @jaredcacurak, I think @KimiZH's idea is probably best. Let client code use the native function in the cases where it works correctly.
However, I think I'd code it differently. We should probably try to use the existing polyfill as much as possible. (If we add more assertions and such to the original polyfill, they'd also apply to this new polyfill.) I'm thinking something like this:
function definePropertyFunctionPrototype (fn, name, descriptor) {
if (name === 'prototype' && typeof fn === 'function') {
// apply shim
return defineProperty(fn, name, descriptor);
}
else {
// use native
return Object.defineProperty(fn, name, descriptor);
}
}
See his commit link in his previous comments for the other half of the change.
Thoughts?
Make sense.
The native implementation of
Object.defineProperties
on Android version >= 2.3.6 && <= 4.0.3 seems to be buggy, where the following test case fails as below, can we check if we need to shim up a bit on those devices?==========Code==========
==========Code==========
==========Result========= Platform APILevel Result 1.5 3 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function 1.6 4 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function 2.1 7 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function 2.2 8 true 2.3.3 10 true 2.3.6 x undefined 4.0 14 undefined 4.0.3 15 undefined 4.1.2 16 true 4.2.2 17 true 4.3 18 true 4.4.2 19 true ==========Result=========