Do not override primordial property by assignment#3109
Conversation
|
Thank you for the pull request. Can you remove the compiled files in docs as we compile them during releases? |
|
Hi @domoritz I did, so we can proceed with this PR. However, this omits the change that matters most, to the line exports.hasOwnProperty = has;This line does not appear anywhere in your sources. I see now that vega does its own code generation, and so must be generating this line. However, I could not figure out where this happens or how to fix it. So I have removed from the PR description that it fixes the |
|
I assigned @jheer as he wrote most of the code. |
|
It looks like the remaining issues stem from vega-util/index.js where we name the exports such as hasOwnProperty. One route could be to rename those exports, though that involves a breaking change and thus a major version bump. Alternatively, perhaps there is a way to signal to package bundlers (such as rollup, which Vega uses) to use defineProperty for those exports? |
|
Is the |
|
Yes, that is the output of rollup. |
|
Thanks. In that case, this PR as is addresses the entire vega specific issue. The remaining bug is in rollup. I'll pursue it over there. Is this PR by itself good? Thanks. |
|
This PR would, therefore, close out #3075 . I will file a separate bug with rollup. |
|
Thanks! |
By "primordial object" I mean all standard EcmaScript objects that exist before code starts running, like especially
Object.prototype. SES and several other systems freeze all JS primordials on initialization. As explained at #3075 , because of the override mistake, on an object that inherits from a primordial object, an attempt to override one of these properties, such ashasOwnProperty, by assignment will fail to do so. (By "override" we mean to create a new own property of the same name on the inheriting object.)This PR replaces the occurrences we are aware of vega making this mistake directly in its source code.
However, as explained at #3075 the cases that matter are automatically generated somehow by vega itself. The change needs to produce code like that show by the patches additions of https://github.com/Agoric/agoric-sdk/blob/master/patches/vega-util%2B1.16.0.patch which enable vega to work under SES as used by the agoric-sdk monorepo.