Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional globals #4949

Merged
merged 1 commit into from
Nov 24, 2017
Merged

Add additional globals #4949

merged 1 commit into from
Nov 24, 2017

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Nov 24, 2017

This adds DTRACE_ globals to the fake global object. This is required to

  1. make a better sandboxing, and
  2. allow future work of re-injecting native modules possible (since things like net.Socket do call these methods).

Methods are wrapped in another file so if people modifies them (e.g. by adding a static property) they will not prevent garbage collection.

@@ -13,10 +13,26 @@ import type {Global} from 'types/Global';
import createProcesObject from './create_process_object';
import deepCyclicCopy from './deep_cyclic_copy';

const DTRACE = [
Copy link
Member

@SimenB SimenB Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any module where this list is maintained? Or can we fish it out from the running global?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do something like Object.keys(global).filter(dtrace => dtrace.startsWith('DTRACE_')) to get it.

export default function(globalObject: Global, globals: ConfigGlobals) {
globalObject.process = createProcesObject();

// Forward some APIs.
DTRACE.forEach(dtrace => {
globalObject['DTRACE_' + dtrace] = function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use ...args and arrow functions instead? arguments is bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a proper function so that this can be captured. Using ...args sounds unnecessary to me, since no processing as Array is needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right about this but ...args should be faster than arguments at this time.

@codecov-io
Copy link

codecov-io commented Nov 24, 2017

Codecov Report

Merging #4949 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4949      +/-   ##
==========================================
- Coverage   60.31%   60.29%   -0.03%     
==========================================
  Files         198      198              
  Lines        6600     6598       -2     
  Branches        4        4              
==========================================
- Hits         3981     3978       -3     
- Misses       2619     2620       +1
Impacted Files Coverage Δ
packages/jest-util/src/install_common_globals.js 100% <100%> (ø) ⬆️
packages/jest-runtime/src/script_transformer.js 85.35% <0%> (-1.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65f8894...d542ea5. Read the comment docs.

globalObject.Buffer = global.Buffer;
globalObject.setImmediate = global.setImmediate;
globalObject.clearImmediate = global.clearImmediate;

Object.assign(global, deepCyclicCopy(globals));
return Object.assign(globalObject, deepCyclicCopy(globals));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 This convinced me to add a test, even if the module appears to be simple.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure CI passes before merging.

@mjesun mjesun merged commit 397acea into jestjs:master Nov 24, 2017
@mjesun mjesun deleted the improve-globals branch December 6, 2017 11:46
};
});

// Forward some others (this breaks the sandbox but for now it's OK).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't presume anyone still remembers this diff, but if you do, I'd love to hear your thoughts on #11204.

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants