Issue 29590603: Issue 5954 - Read-only properties cannot be assigned in strict mode in Edge (Closed)
|
Created:
Oct. 27, 2017, 1:10 p.m. by Oleksandr Modified:
Nov. 13, 2017, 11:35 a.m. Visibility:
Public. |
DescriptionIssue 5954 - Read-only properties cannot be assigned in strict mode in Edge
Patch Set 1 #
Total comments: 5
Patch Set 2 : Refactor. Mention accessor descriptors. #
Total comments: 10
Patch Set 3 : Use function instead of arrow function. #Patch Set 4 : Delete accessor methods explicitly #
Total comments: 1
Patch Set 5 : Remove the comment #MessagesTotal messages: 15
https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js#newcode72 polyfill.js:72: let descriptor = Reflect.getOwnPropertyDescriptor(object, name); We cannot use Object.getOwnPropertyDescriptor because it is not defined for some objects. https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js#newcode74 polyfill.js:74: if (descriptor && descriptor.configurable) There isn't really an object in our list that doesn't satisfy these requirements, so maybe this check can be removed.
https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js#newcode72 polyfill.js:72: let descriptor = Reflect.getOwnPropertyDescriptor(object, name); On 2017/10/27 13:12:37, Oleksandr wrote: > We cannot use Object.getOwnPropertyDescriptor because it is not defined for some > objects. We can use Object.getOwnPropertyDescriptor here since we know for sure that the target won't be undefined. Nit: I would add a blank line after the return statement above this, but your choice. https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js#newcode74 polyfill.js:74: if (descriptor && descriptor.configurable) On 2017/10/27 13:12:38, Oleksandr wrote: > There isn't really an object in our list that doesn't satisfy these > requirements, so maybe this check can be removed. Yes, we should remove it. If it's not configurable it'll fail during development and we'll know we need to figure something out for it. https://codereview.adblockplus.org/29590603/diff/29590604/polyfill.js#newcode76 polyfill.js:76: Object.defineProperty(object, name, { You could just do: descriptor.value = (...args) => { ... }; Object.defineProperty(object, name, descriptor); This way we don't have to hard-code the values for enumerable, writable, and configurable, they will be what they are on that platform.
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode80 polyfill.js:80: descriptor.value = (...args) => Functions and arrow functions have different semantics. Since this is an API wrapper here, I think maybe it's better to make it a normal function.
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); How about this (untested): let descriptor = Object.getOwnPropertyDescriptor(object, name); descriptor.value = function(...args) { ... https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode80 polyfill.js:80: descriptor.value = (...args) => On 2017/11/05 10:13:52, Manish Jethani wrote: > Functions and arrow functions have different semantics. Since this is an API > wrapper here, I think maybe it's better to make it a normal function. Agreed
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); On 2017/11/06 08:52:01, kzar wrote: > How about this (untested): > > let descriptor = Object.getOwnPropertyDescriptor(object, name); > descriptor.value = function(...args) > { > ... This doesn't work, because some descriptors can be accessor descriptors. That means they will have set() and/or get() methods, but no value(). If we also set value() then a call to defineProperty() will fail, because descriptors cannot be of both accessor and data types https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode80 polyfill.js:80: descriptor.value = (...args) => On 2017/11/05 10:13:52, Manish Jethani wrote: > Functions and arrow functions have different semantics. Since this is an API > wrapper here, I think maybe it's better to make it a normal function. Done.
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); On 2017/11/06 09:05:00, Oleksandr wrote: > On 2017/11/06 08:52:01, kzar wrote: > > How about this (untested): > > > > let descriptor = Object.getOwnPropertyDescriptor(object, name); > > descriptor.value = function(...args) > > { > > ... > > This doesn't work, because some descriptors can be accessor descriptors. That > means they will have set() and/or get() methods, but no value(). If we also set > value() then a call to defineProperty() will fail, because descriptors cannot be > of both accessor and data types > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... Which APIs are returned using a getter? I find it surprising that any would be.
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); On 2017/11/06 10:12:38, kzar wrote: > On 2017/11/06 09:05:00, Oleksandr wrote: > > On 2017/11/06 08:52:01, kzar wrote: > > > How about this (untested): > > > > > > let descriptor = Object.getOwnPropertyDescriptor(object, name); > > > descriptor.value = function(...args) > > > { > > > ... > > > > This doesn't work, because some descriptors can be accessor descriptors. That > > means they will have set() and/or get() methods, but no value(). If we also > set > > value() then a call to defineProperty() will fail, because descriptors cannot > be > > of both accessor and data types > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... > > Which APIs are returned using a getter? I find it surprising that any would be. For example `setUnintstallUrl`, as it says on the line below ;)
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); On 2017/11/06 10:14:18, Oleksandr wrote: > On 2017/11/06 10:12:38, kzar wrote: > > On 2017/11/06 09:05:00, Oleksandr wrote: > > > On 2017/11/06 08:52:01, kzar wrote: > > > > How about this (untested): > > > > > > > > let descriptor = Object.getOwnPropertyDescriptor(object, name); > > > > descriptor.value = function(...args) > > > > { > > > > ... > > > > > > This doesn't work, because some descriptors can be accessor descriptors. > That > > > means they will have set() and/or get() methods, but no value(). If we also > > set > > > value() then a call to defineProperty() will fail, because descriptors > cannot > > be > > > of both accessor and data types > > > > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... > > > > Which APIs are returned using a getter? I find it surprising that any would > be. > > For example `setUnintstallUrl`, as it says on the line below ;) Ah right, well you could just delete the getter and setters. let descriptor = Object.getOwnPropertyDescriptor(object, name); delete descriptor["get"]; delete descriptor["set"]; descriptor.value = function(...args) { ... This also makes me wonder why did they use a getter? Could the function change at some point in the future, in which case breaking our wrapper?
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); > This also makes me wonder why did they use a getter? Could the function change > at some point in the future, in which case breaking our wrapper? I guess we'll just have to see. I don't know why they used a getter.
Otherwise LGTM https://codereview.adblockplus.org/29590603/diff/29601587/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29601587/polyfill.js#newcode73 polyfill.js:73: // Some descriptors like setUninstallURL are in fact accessor descriptors. Nit: This comment doesn't read too well and I don't think it adds much, maybe just remove it?
https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js File polyfill.js (right): https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, name); On 2017/11/08 13:58:09, Oleksandr wrote: > > > This also makes me wonder why did they use a getter? Could the function change > > at some point in the future, in which case breaking our wrapper? > > I guess we'll just have to see. I don't know why they used a getter. What do you think about that Manish?
On 2017/11/10 14:18:20, kzar wrote: > https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js > File polyfill.js (right): > > https://codereview.adblockplus.org/29590603/diff/29597564/polyfill.js#newcode72 > polyfill.js:72: let oldDescriptor = Object.getOwnPropertyDescriptor(object, > name); > On 2017/11/08 13:58:09, Oleksandr wrote: > > > > > This also makes me wonder why did they use a getter? Could the function > change > > > at some point in the future, in which case breaking our wrapper? > > > > I guess we'll just have to see. I don't know why they used a getter. > > What do you think about that Manish? I think it's probably OK to just replace the accessors with just a property the way it's being done here. If there's a problem, we'll just have to wait to find out. I agree that the comment doesn't add much, it's even Edge-specific (but doesn't mention this). I would remove it too, it is kind of self-explanatory what's going on there.
LGTM 
|
