realm.js: don't freeze globals #104
Conversation
|
My apologies for not yet adding a test: I need to get a node-10 environment installed so I can run the test suite locally. We need something which creates a new realm and then attempts to replace one of the existing globals, i.e. |
|
@ljharb thanks! I'll give that a try. |
|
Ah, it looks like current master doesn't pass tests on node7/8/9 (probably due to a |
|
I pushed a new commit that adds tests, which revealed that I had the exclusion logic wrong, so that's been fixed too. I ran the tests successfully on a different branch, which omitted the most recent commit (3ccc45a). This PR will fail CI until that gets fixed. |
shim/src/realm.js
Outdated
| @@ -89,7 +89,7 @@ function setDefaultBindings(realmRec) { | |||
| const intrinsics = realmRec[Intrinsics]; | |||
| const descs = getStdLib(intrinsics); | |||
| for (const name of Object.getOwnPropertyNames(descs)) { | |||
| if (name !== 'Infinity' || name !== 'NaN' || name !== 'undefined') { | |||
| if (!(name in { Infinity, NaN, undefined })) { | |||
There was a problem hiding this comment.
the previous check was better; this will incorrectly pass for toString, for example.
There was a problem hiding this comment.
Good point! (My bad)
In any case, the previous code had a deMorgan confusion. It needed to use && rather than ||. That's what we should return to.
warner
changed the title
|
I'm going to rebase this onto current master, to provoke travis into doing a new build. |
|
Ah, good, it's passing now. Let me know if you'd prefer the writable+configurable settings to live in |
|
BTW, https://github.com/agoric/proposal-realms/tree/unfreeze-globals-2 has an extra commit to move the settings to |
|
This patch uses the list-of-names approach, as @jfparadis suggested today. There's one outstanding question: should |
The spec says the initial Realm state should have writable+configurable global properties (except for three). We'll want this to happen for frozen/SES realms, eventually, but not here. We need to be able to run a shim that replaces Realm, or others.
The spec says the initial Realm state should have writable+configurable
global properties (except for three).
We'll want this to happen for frozen/SES realms, eventually, but not here. We
need to be able to run a shim that replaces Realm, or others.