Rewrite how common globals are installed in "global" #4904
Conversation
| value = obj[key]; | ||
| if (typeof value === 'object' && value !== null) { | ||
| value = deepCopy(value); | ||
| if (obj.hasOwnProperty(key)) { |
| [Symbol.toStringTag]: 'process', | ||
| }; | ||
| export default (globalObject: Global, globals: ConfigGlobals) => { | ||
| let prototype = Object.getPrototypeOf(process); |
There was a problem hiding this comment.
Based on https://github.com/nodejs/node/blob/master/lib/process.js I'm assuming there is no other way to get the prototype.
| globalObject.Buffer = global.Buffer; | ||
|
|
||
| // Make a copy of "process" to avoid memory leaks. | ||
| if (typeof Symbol !== 'undefined' && Symbol.toStringTag) { |
There was a problem hiding this comment.
I think we can remove the if statement. We now only support versions of node that have symbol and we can assume Symbol.toStringTag is available.
| } | ||
| } | ||
|
|
||
| globalObject.process = processObject; |
There was a problem hiding this comment.
Let's factor all the process code out into a separate function and do globalObject.process = createProcessObject(process)?
|
|
||
| // Copy all remaining properties that are not already in the object. | ||
| for (const key in process) { | ||
| if (process.hasOwnProperty(key) && !processObject.hasOwnProperty(key)) { |
There was a problem hiding this comment.
Good direction. I think you'll also have to patch jest-runtime to make it so that require('process') will return this process object rather than giving you access to the real process.
People might have code that does process === require('process') and this diff would break it. Please also add an integration test that ensures these two are the same.
|
|
||
| import deepCyclicCopy from '../deep_cyclic_copy'; | ||
|
|
||
| it('returns the same thing for primitive or function values', () => { |
| import deepCyclicCopy from './deep_cyclic_copy'; | ||
|
|
||
| export default function() { | ||
| const newProcess = deepCyclicCopy(process); |
There was a problem hiding this comment.
How about in this file we do const process = require('process')? Seems nicer.
| newProcess[Symbol.toStringTag] = 'process'; | ||
|
|
||
| // Sequentially execute all constructors over the object. | ||
| for (let chain = process; chain; chain = Object.getPrototypeOf(chain)) { |
There was a problem hiding this comment.
Woah what the hell is this for loop. What about do-while?
There was a problem hiding this comment.
You don't have to change it, it's just very confusing to see "chain;" in there, took me a second :D
There was a problem hiding this comment.
Let me change it; I don't want confusing code and especially in that file :)
| * @flow | ||
| */ | ||
|
|
||
| export default function deepCyclicCopy( |
There was a problem hiding this comment.
Great function but oh man, it's so hard to clone stuff + it's slow in JS :(
|
@cpojer I had to add a small hack to make Jasmine work asynchronously. Jasmine attaches itself to The proposed solution should not create a leak. The leak with A proper fix would be to execute all JS outside the context; and just expose |
Codecov Report
@@ Coverage Diff @@
## master #4904 +/- ##
==========================================
+ Coverage 59.77% 59.89% +0.11%
==========================================
Files 195 197 +2
Lines 6535 6547 +12
Branches 3 3
==========================================
+ Hits 3906 3921 +15
+ Misses 2629 2626 -3
Continue to review full report at Codecov.
|
|
Instead of introducing another global, let's pass "process" from the parent context into the Jasmine factory here: https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/index.js#L39 and thread it through to the place that needs the real process. I'm sorry if this requires a bit more of a refactor but adding a global variable just for this isn't the right way to go about it (I appreciate the hack to unblock though). |
|
@cpojer commit amended, thanks a lot for the suggestion! Now it looks way better 🙂 |
|
This is two million times better. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This diff rewrites how the fake
processvariable is created and injected; in order to better mimic the way it looks and improve sandboxing. Now,processis built fromProcess.prototype, then all the constructors on the prototype chain are executed (i.e.Process,EventEmitter, andObject); and finally the remaining properties are copied as before.This gives as a result an object that can independently handle events added via
process.on, but also passes validations likeObject.getPrototypeOf(process) !== Object.prototype.The risk is that someone is already using
process.onto try attaching to the realprocess; and this will make the test break. But I think it's fine.