Leave global scope untouched
bojavou opened this issue · 5 comments
- FakeTimers version : latest, 11.0.0
- Environment : Linux
- Example URL : https://github.com/bojavou/fake-timers-crash
- Other libraries you are using: None
What did you expect to happen?
The library loads without mutating the global scope.
What actually happens
The library attempts to mutate the global scope on load.
How to reproduce
This can be seen with the demo repo.
$ git clone https://github.com/bojavou/fake-timers-crash.git
$ cd fake-timers-crash
$ npm install
$ node load.js
/home/user/demo/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:204
_global.setImmediate = _global.setImmediate;
^
TypeError: Cannot assign to read only property 'setImmediate' of object '#<Object>'
at withGlobal (/home/user/demo/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:204:30)
at Object.<anonymous> (/home/user/demo/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:1811:31)
at Module._compile (node:internal/modules/cjs/loader:1275:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1329:10)
at Module.load (node:internal/modules/cjs/loader:1133:32)
at Module._load (node:internal/modules/cjs/loader:972:12)
at Module.require (node:internal/modules/cjs/loader:1157:19)
at require (node:internal/modules/helpers:119:18)
at Object.<anonymous> (/home/user/demo/fake-timers-crash/load.js:5:1)
at Module._compile (node:internal/modules/cjs/loader:1275:14)
Discussion
I'm running in a locked down environment with global properties frozen. This causes the library to crash. I see it through loading sinon
in tests. I'd prefer if the library didn't touch the global scope unless I requested it.
On load there's a defaultImplementation
constructed around the global scope.
const defaultImplementation = withGlobal(globalObject);
The withGlobal()
function writes to setImmediate
and clearImmediate
.
if (setImmediatePresent) {
_global.setImmediate = _global.setImmediate;
_global.clearImmediate = _global.clearImmediate;
}
I'm not sure what the intention here is. The value is read and assigned to the same place. These look like they should be noops to me, but maybe there's some setter magic. Could these be safely removed? I'm willing to submit a patch if someone knows the right solution.
You can have a look at the git log for those lines (originally from Sinon, so might be in Sinon 1.5 or something like that), but AFAIK this is a workaround for an old peculiarity with Internet Explorer. You needed to do this seemingly pointless song and dance to be able to mutate it later (or something of that sorts).
As we no longer support IE, only evergreen browsers, this and other hacks like it in Sinon or associated libraries, can be removed by anyone willing to provide the PR. So feel free ✌️
I think this was fixed in #477 ?
Still need to release a new version, though :)
Verified to have been fixed:
$ bash testscript.sh
Cloning into 'fake-timers-crash'...
remote: Enumerating objects: 4, done.
remote: Counting objects: 100% (4/4), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 4 (delta 0), reused 4 (delta 0), pack-reused 0
Receiving objects: 100% (4/4), done.
added 3 packages, and audited 4 packages in 2s
found 0 vulnerabilities
/Users/carlerik/dev/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:204
_global.setImmediate = _global.setImmediate;
^
TypeError: Cannot assign to read only property 'setImmediate' of object '#<Object>'
at withGlobal (/Users/carlerik/dev/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:204:30)
at Object.<anonymous> (/Users/carlerik/dev/fake-timers-crash/node_modules/@sinonjs/fake-timers/src/fake-timers-src.js:1811:31)
at Module._compile (node:internal/modules/cjs/loader:1233:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1287:10)
at Module.load (node:internal/modules/cjs/loader:1091:32)
at Module._load (node:internal/modules/cjs/loader:938:12)
at Module.require (node:internal/modules/cjs/loader:1115:19)
at require (node:internal/modules/helpers:119:18)
at Object.<anonymous> (/Users/carlerik/dev/fake-timers-crash/load.js:5:1)
at Module._compile (node:internal/modules/cjs/loader:1233:14)
Node.js v20.5.0
carlerik at MacBook-Pro in ~/dev
$ z fake
carlerik at MacBook-Pro in ~/dev/fake-timers (main)
$ npm link
added 1 package, and audited 3 packages in 1s
found 0 vulnerabilities
Reshimming asdf nodejs...
carlerik at MacBook-Pro in ~/dev/fake-timers (main)
$ cd ../fake-timers-crash/
carlerik at MacBook-Pro in ~/dev/fake-timers-crash (main)
$ cat package.json
{
"dependencies": {
"@sinonjs/fake-timers": "^11.0.0"
}
}
carlerik at MacBook-Pro in ~/dev/fake-timers-crash (main)
$ npm link @sinonjs/fake-timers
removed 2 packages, changed 1 package, and audited 3 packages in 763ms
found 0 vulnerabilities
carlerik at MacBook-Pro in ~/dev/fake-timers-crash (main)
$ cat ../testscript.sh
git clone https://github.com/bojavou/fake-timers-crash.git
cd fake-timers-crash
npm install
node load.js
carlerik at MacBook-Pro in ~/dev/fake-timers-crash (main)
$ node load.js
Released as 11.1.0