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

Re-inject native Node modules #4970

Merged
merged 4 commits into from Nov 29, 2017

Conversation

Projects
None yet
6 participants
@mjesun
Contributor

mjesun commented Nov 27, 2017

This PR improves the sandboxing for native modules. Every time a native require is detected, Jest will:

  • introduce it inside the sandbox by re-evaluating its source code, exposed through process.binding('natives'); which differs from the previous approach where require(moduleName) was done on the main context; and
  • switch the require logic to know it's inside a native module (thus, internal/util will not resolve to the NPM internal module, but to the native Node one).

This kills all memory leaks related to process and native module modification. I have added an integration test with the most common use cases. Executing the test (./jest leak_detection) on a previous commit will fail because of detected leaks.

It also makes the jest test suite leak free itself (tried with ./jest -i --detectLeaks packages).

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 27, 2017

Collaborator

Does this fix #4630?

Collaborator

SimenB commented Nov 27, 2017

Does this fix #4630?

@mjesun

This comment has been minimized.

Show comment
Hide comment
@mjesun

mjesun Nov 27, 2017

Contributor

Nope; but I'll work on the child one as soon as I can. I'm very curious to see why this test breaks 🙂

Contributor

mjesun commented Nov 27, 2017

Nope; but I'll work on the child one as soon as I can. I'm very curious to see why this test breaks 🙂

const result = runJest.json('require-all-modules').json;
if (!result.success) {
console.warn(result);

This comment has been minimized.

@SimenB

SimenB Nov 27, 2017

Collaborator

for debugging? should it be removed?

@SimenB

SimenB Nov 27, 2017

Collaborator

for debugging? should it be removed?

This comment has been minimized.

@mjesun

mjesun Nov 27, 2017

Contributor

I added it so that if a module throws when required, we can know who was based on the console 🙂

@mjesun

mjesun Nov 27, 2017

Contributor

I added it so that if a module throws when required, we can know who was based on the console 🙂

@@ -0,0 +1,9 @@
#!/usr/bin/env node --inspect-brk

This comment has been minimized.

@SimenB

SimenB Nov 27, 2017

Collaborator

fancy :D

@SimenB

SimenB Nov 27, 2017

Collaborator

fancy :D

runtime: Runtime,
testPath: string,
): Promise<TestResult> {
// The "environment" parameter is nullable just so that we can clean its

This comment has been minimized.

@SimenB

SimenB Nov 27, 2017

Collaborator

will this help pinpointing #4710?

@SimenB

SimenB Nov 27, 2017

Collaborator

will this help pinpointing #4710?

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 27, 2017

Collaborator

Failing on appveyor 🙁

Collaborator

SimenB commented Nov 27, 2017

Failing on appveyor 🙁

@mjesun

This comment has been minimized.

Show comment
Hide comment
@mjesun

mjesun Nov 27, 2017

Contributor

Yes, I just debugged it. What is happening is that jasmine-reporters/src/appveyor_reporter.js is injected inside the VM context (see here), but it is called after the test finishes. By then, the environment has been cleaned and the reporter fails.

To be precise, what fails is the execution of any require within the VM context after the test suite has finished. The appveyor_reporter.js has an inline require to http, which is the one breaking.

IMO this reporters should be injected outside the VM context (i.e. on the main context); but I wonder if doing so can create other undesirable side effects. cc @cpojer, @SimenB, @thymikee.

Contributor

mjesun commented Nov 27, 2017

Yes, I just debugged it. What is happening is that jasmine-reporters/src/appveyor_reporter.js is injected inside the VM context (see here), but it is called after the test finishes. By then, the environment has been cleaned and the reporter fails.

To be precise, what fails is the execution of any require within the VM context after the test suite has finished. The appveyor_reporter.js has an inline require to http, which is the one breaking.

IMO this reporters should be injected outside the VM context (i.e. on the main context); but I wonder if doing so can create other undesirable side effects. cc @cpojer, @SimenB, @thymikee.

@thymikee

This comment has been minimized.

Show comment
Hide comment
@thymikee

thymikee Nov 27, 2017

Collaborator

I guess it makes sense to create another hook which works outside of VM context and put jasmine reporters there.

Collaborator

thymikee commented Nov 27, 2017

I guess it makes sense to create another hook which works outside of VM context and put jasmine reporters there.

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Nov 28, 2017

Contributor

Yeah, we'll need to fix that issue first before we can merge this PR.

Contributor

cpojer commented Nov 28, 2017

Yeah, we'll need to fix that issue first before we can merge this PR.

@@ -25,10 +25,17 @@ const JASMINE = require.resolve('./jasmine/jasmine_light.js');
async function jasmine2(
globalConfig: GlobalConfig,
config: ProjectConfig,
environment: Environment,
environment: ?Environment,

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

woah, let's not do this please. For example by renaming the environment variable below?

@cpojer

cpojer Nov 28, 2017

Contributor

woah, let's not do this please. For example by renaming the environment variable below?

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

We have to; I need to kill all environment references.

@mjesun

mjesun Nov 28, 2017

Contributor

We have to; I need to kill all environment references.

if (config.timers === 'fake') {
environment.fakeTimers.useFakeTimers();
}
}
});
// Free references to environment to avoid leaks.
env.afterAll(() => {
environment = null;

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

Who is holding on to environment?

@cpojer

cpojer Nov 28, 2017

Contributor

Who is holding on to environment?

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

The beforeAll closure, through environment.fakeTimers.useFakeTimers().

@mjesun

mjesun Nov 28, 2017

Contributor

The beforeAll closure, through environment.fakeTimers.useFakeTimers().

import createProcessObject from '../create_process_object';
it('creates a process object that looks like the original one', () => {
const fakeProcess = createProcessObject();
// "process" inherits from EventEmitter through the prototype chain.
expect(fakeProcess instanceof EventEmitter).toBe(true);

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

Hah, I think this is a bug actually. You are creating the process object with EventEmitter from the parent but it should use runtime._requireNativeModule('events') so that this test still passes. Changing this test isn't the right fix here imho.

@cpojer

cpojer Nov 28, 2017

Contributor

Hah, I think this is a bug actually. You are creating the process object with EventEmitter from the parent but it should use runtime._requireNativeModule('events') so that this test still passes. Changing this test isn't the right fix here imho.

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

Exactly; I explained that to @thymikee before. The issue here is that in order to get the child EventEmitter I need to execute in the context require('events').EventEmitter. Unfortunately at this point neither the context, nor the require implementation are ready; so it's too complex to add the change in this PR.

Since I just re-did the process object about a week ago, and before it did not inherit from EventEmitter either, I think we are more or less safe to do this.

However, at the core of the issue, you are both right, the check should pass 🙂

@mjesun

mjesun Nov 28, 2017

Contributor

Exactly; I explained that to @thymikee before. The issue here is that in order to get the child EventEmitter I need to execute in the context require('events').EventEmitter. Unfortunately at this point neither the context, nor the require implementation are ready; so it's too complex to add the change in this PR.

Since I just re-did the process object about a week ago, and before it did not inherit from EventEmitter either, I think we are more or less safe to do this.

However, at the core of the issue, you are both right, the check should pass 🙂

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

In that case let's change where we are initializing process then? It seems like that should happen later or lazily. You could make process a lazy getter that, when first accessed, overwrites itself by doing global.process = process. What do you think about that? We just need to make sure there is nothing in events or its dependencies that depends on process which would produce a circular dependency.

@cpojer

cpojer Nov 28, 2017

Contributor

In that case let's change where we are initializing process then? It seems like that should happen later or lazily. You could make process a lazy getter that, when first accessed, overwrites itself by doing global.process = process. What do you think about that? We just need to make sure there is nothing in events or its dependencies that depends on process which would produce a circular dependency.

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

I tried making this work, but I ended up with a V8 crash (see below). I think we should revisit it later; but it should be kept out of the scope of this PR.

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 3: v8::V8::ToLocalEmpty() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 4: node::(anonymous namespace)::ContextifyScript::RunInContext(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 8: 0x2fbc95e0463d
 9: 0x2fbc95ef4048
@mjesun

mjesun Nov 28, 2017

Contributor

I tried making this work, but I ended up with a V8 crash (see below). I think we should revisit it later; but it should be kept out of the scope of this PR.

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
 1: node::Abort() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 3: v8::V8::ToLocalEmpty() [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 4: node::(anonymous namespace)::ContextifyScript::RunInContext(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/.../.nvm/versions/node/v8.9.1/bin/node]
 8: 0x2fbc95e0463d
 9: 0x2fbc95ef4048

This comment has been minimized.

@cpojer

cpojer Nov 29, 2017

Contributor

Can you file a task to fix this?

@cpojer

cpojer Nov 29, 2017

Contributor

Can you file a task to fix this?

const isNativeModule =
moduleName &&
((options && options.isNativeModule) ||
this._resolver.isCoreModule(moduleName));

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

woah.. pretty :P

@cpojer

cpojer Nov 28, 2017

Contributor

woah.. pretty :P

if (isNativeModule) {
moduleRegistry = this._nativeModuleRegistry;
} else if (options && options.isInternalModule) {
moduleRegistry = this._internalModuleRegistry;

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

The internal module registry is used in Jest for Jest's own core modules like pretty-format or chalk. Any reason you can't reuse that one? Why do you need a third one?

@cpojer

cpojer Nov 28, 2017

Contributor

The internal module registry is used in Jest for Jest's own core modules like pretty-format or chalk. Any reason you can't reuse that one? Why do you need a third one?

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

Because of clashes. Once you start requiring inside a Jest internal, all the child modules are required inside Jest are internal modules as well (which is fine, because you prevent clashes between test and Jest code).

If Jest requires internal/util, internal refers to NPM's module; but if you require it from the Native util module, it refers to the internal file; so you need a separate require implementation (as well as a separate registry) to avoid the conflict.

@mjesun

mjesun Nov 28, 2017

Contributor

Because of clashes. Once you start requiring inside a Jest internal, all the child modules are required inside Jest are internal modules as well (which is fine, because you prevent clashes between test and Jest code).

If Jest requires internal/util, internal refers to NPM's module; but if you require it from the Native util module, it refers to the internal file; so you need a separate require implementation (as well as a separate registry) to avoid the conflict.

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

Ok, that makes sense.

@cpojer

cpojer Nov 28, 2017

Contributor

Ok, that makes sense.

Object.defineProperty(localModule, 'require', {
value: this._createRequireImplementation(filename, options),
});
const fileSource = isNativeModule
? // $FlowFixMe: process.binding exists.
process.binding('natives')[filename]

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

is this call fast or slow?

@cpojer

cpojer Nov 28, 2017

Contributor

is this call fast or slow?

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

Really fast. The following code:

console.time('binding');

for (let i = 0; i < 100000; i++) {
  global.fsSourceCode = process.binding('natives').fs;
}

console.timeEnd('binding');

Reports an average of 16ms per 100,000 calls.

@mjesun

mjesun Nov 28, 2017

Contributor

Really fast. The following code:

console.time('binding');

for (let i = 0; i < 100000; i++) {
  global.fsSourceCode = process.binding('natives').fs;
}

console.timeEnd('binding');

Reports an average of 16ms per 100,000 calls.

moduleRegistry: ModuleRegistry,
from: Path,
) {
switch (moduleName) {

This comment has been minimized.

@cpojer

cpojer Nov 28, 2017

Contributor

I like it.

@cpojer

cpojer Nov 28, 2017

Contributor

I like it.

@cpojer

This is great work, I'm so excited for better sandboxing and fewer memory leaks in Jest. Can you work on some of the inline comments that I left?

Before merging this, here are some more high-level questions that would be great to answer:

  • Can you show results of --logHeapUsage -i before and after this change?
  • How does this affect performance of Jest?
  • Can we reduce this to only two module registries (internal and user) vs. three?
@@ -33,7 +33,7 @@ export type Options = {|
collectCoverage: boolean,
collectCoverageFrom: Array<Glob>,
collectCoverageOnlyFrom: ?{[key: string]: boolean, __proto__: null},
isCoreModule?: boolean,
isNativeModule?: boolean,

This comment has been minimized.

@SimenB

SimenB Nov 28, 2017

Collaborator

why change the name? "native module" in my mind is native addons (https://nodejs.org/api/addons.html), not node core modules.

I suppose it's not public API, but still

@SimenB

SimenB Nov 28, 2017

Collaborator

why change the name? "native module" in my mind is native addons (https://nodejs.org/api/addons.html), not node core modules.

I suppose it's not public API, but still

This comment has been minimized.

@mjesun

mjesun Nov 28, 2017

Contributor

We already use a isCoreModule in packages/jest-resolve/src/index.js, which does not quite match the meaning of isNativeModule. When I noticed it I decided to rename all the new additions.

@mjesun

mjesun Nov 28, 2017

Contributor

We already use a isCoreModule in packages/jest-resolve/src/index.js, which does not quite match the meaning of isNativeModule. When I noticed it I decided to rename all the new additions.

@mjesun

This comment has been minimized.

Show comment
Hide comment
@mjesun

mjesun Nov 28, 2017

Contributor
  • Can you show results of --logHeapUsage -i before and after this change?
    Yes, heap consumption grows by approx. a 30%. This is expected, since heap measurement is done before we clean the environment; and now all native modules are re-evaluated.

  • How does this affect performance of Jest?
    Time wise, I could see some inconsistent performance regression of around 10%. Second runs with the change take longer (measured on the same machine). Maybe @thymikee could also run against his test suite, so we get other numbers 🙂

  • Can we reduce this to only two module registries (internal and user) vs. three?
    Unfortunately no 😞, Jest's and Node's internal/util mean different things, so we have to address that to avoid key collisions).

Contributor

mjesun commented Nov 28, 2017

  • Can you show results of --logHeapUsage -i before and after this change?
    Yes, heap consumption grows by approx. a 30%. This is expected, since heap measurement is done before we clean the environment; and now all native modules are re-evaluated.

  • How does this affect performance of Jest?
    Time wise, I could see some inconsistent performance regression of around 10%. Second runs with the change take longer (measured on the same machine). Maybe @thymikee could also run against his test suite, so we get other numbers 🙂

  • Can we reduce this to only two module registries (internal and user) vs. three?
    Unfortunately no 😞, Jest's and Node's internal/util mean different things, so we have to address that to avoid key collisions).

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Nov 28, 2017

Contributor

Huh, can you try to run again with node --expose-gc? That will run the garbage collector before logging heap usage. A 30% increase in memory usage goes counter to what this diff is aiming to solve.

Contributor

cpojer commented Nov 28, 2017

Huh, can you try to run again with node --expose-gc? That will run the garbage collector before logging heap usage. A 30% increase in memory usage goes counter to what this diff is aiming to solve.

@mjesun

This comment has been minimized.

Show comment
Hide comment
@mjesun

mjesun Nov 28, 2017

Contributor

As explained, it is expected based on where the memory is being measured.

Contributor

mjesun commented Nov 28, 2017

As explained, it is expected based on where the memory is being measured.

@mjesun

This comment has been minimized.

Show comment
Hide comment
@mjesun

mjesun Nov 28, 2017

Contributor

Added a new commit (also rebased over master); modifying where memory is measured. Memory consumption went down from 156 to 143 MB (i.e ~10% less memory 🙂) while measured in Jest repository.

However, the memory still grows within the first tests, up until it stabilizes around 143 MB. Some simple snapshots between tests (running with -i) show that part of the increase comes from objects storing compiled source code (i.e. cacheFS), but there are many others.

Contributor

mjesun commented Nov 28, 2017

Added a new commit (also rebased over master); modifying where memory is measured. Memory consumption went down from 156 to 143 MB (i.e ~10% less memory 🙂) while measured in Jest repository.

However, the memory still grows within the first tests, up until it stabilizes around 143 MB. Some simple snapshots between tests (running with -i) show that part of the increase comes from objects storing compiled source code (i.e. cacheFS), but there are many others.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 28, 2017

Codecov Report

Merging #4970 into master will increase coverage by 0.01%.
The diff coverage is 61.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4970      +/-   ##
==========================================
+ Coverage   60.25%   60.27%   +0.01%     
==========================================
  Files         198      198              
  Lines        6623     6659      +36     
  Branches        4        3       -1     
==========================================
+ Hits         3991     4014      +23     
- Misses       2632     2645      +13
Impacted Files Coverage Δ
packages/jest-runner/src/run_test.js 1.96% <0%> (-0.27%) ⬇️
packages/jest-jasmine2/src/index.js 5.47% <0%> (-0.41%) ⬇️
packages/jest-runtime/src/script_transformer.js 86.5% <100%> (ø) ⬆️
packages/jest-util/src/index.js 100% <100%> (+11.76%) ⬆️
packages/jest-util/src/install_common_globals.js 100% <100%> (ø) ⬆️
packages/jest-message-util/src/index.js 86.13% <66.66%> (-0.6%) ⬇️
packages/jest-runtime/src/index.js 74.83% <86.04%> (+1.2%) ⬆️

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 0338a82...cf83dff. Read the comment docs.

codecov-io commented Nov 28, 2017

Codecov Report

Merging #4970 into master will increase coverage by 0.01%.
The diff coverage is 61.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4970      +/-   ##
==========================================
+ Coverage   60.25%   60.27%   +0.01%     
==========================================
  Files         198      198              
  Lines        6623     6659      +36     
  Branches        4        3       -1     
==========================================
+ Hits         3991     4014      +23     
- Misses       2632     2645      +13
Impacted Files Coverage Δ
packages/jest-runner/src/run_test.js 1.96% <0%> (-0.27%) ⬇️
packages/jest-jasmine2/src/index.js 5.47% <0%> (-0.41%) ⬇️
packages/jest-runtime/src/script_transformer.js 86.5% <100%> (ø) ⬆️
packages/jest-util/src/index.js 100% <100%> (+11.76%) ⬆️
packages/jest-util/src/install_common_globals.js 100% <100%> (ø) ⬆️
packages/jest-message-util/src/index.js 86.13% <66.66%> (-0.6%) ⬇️
packages/jest-runtime/src/index.js 74.83% <86.04%> (+1.2%) ⬆️

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 0338a82...cf83dff. Read the comment docs.

};
let runtime = new Runtime(
config,

This comment has been minimized.

@cpojer

cpojer Nov 29, 2017

Contributor

I think it's time to turn this constructor into something that takes an object.

@cpojer

cpojer Nov 29, 2017

Contributor

I think it's time to turn this constructor into something that takes an object.

This comment has been minimized.

@mjesun

mjesun Nov 29, 2017

Contributor

Should we make this change now? This would break Runtime's interface; that's why I thought it was better to keep it as-is.

@mjesun

mjesun Nov 29, 2017

Contributor

Should we make this change now? This would break Runtime's interface; that's why I thought it was better to keep it as-is.

This comment has been minimized.

@cpojer

cpojer Nov 29, 2017

Contributor

No, that should be a follow-up but we could do it for Jest 22.

@cpojer

cpojer Nov 29, 2017

Contributor

No, that should be a follow-up but we could do it for Jest 22.

@cpojer

cpojer approved these changes Nov 29, 2017

Awesome. Please rebase + remove the merged PR + fix some of the nits before merging. I'm still awaiting the memory difference of this on the two large codebases internally, mind sending me that within FB? :)

mjesun and others added some commits Nov 27, 2017

@cpojer cpojer merged commit ef55e89 into facebook:master Nov 29, 2017

2 of 5 checks passed

ci/circleci: test-node-6 CircleCI is running your tests
Details
ci/circleci: test-node-8 CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 29, 2017

Collaborator

This should probably have an entry in the changelog

Collaborator

SimenB commented Nov 29, 2017

This should probably have an entry in the changelog

@mjesun mjesun deleted the mjesun:re-inject-native-modules branch Dec 4, 2017

mjesun added a commit that referenced this pull request Dec 4, 2017

mjesun added a commit that referenced this pull request Dec 4, 2017

cpojer added a commit that referenced this pull request Dec 4, 2017

Revert "Re-inject native Node modules" (#5009)
* Revert "docs: update expect.anything() example (#5007)"

This reverts commit ea3fabc.

* Revert "Emphasise required return (#4999)"

This reverts commit 4f1113c.

* Revert "Makes NPM a link like Yarn is (#4998)"

This reverts commit aa4f09d.

* Revert "Update Timeout error message to `jest.timeout` and display current timeout value (#4990)"

This reverts commit 08f8394.

* Revert "fix: jest-util should not depend on jest-mock (#4992)"

This reverts commit 4e2f41f.

* Revert "Update Troubleshooting.md (#4988)"

This reverts commit 6414f28.

* Revert "Revert "Add the Yammer logo to the 'who is using this' section of the website." (#4987)"

This reverts commit 91b104f.

* Revert "Make "weak" optional dependency and check it at runtime (#4984)"

This reverts commit e00529d.

* Revert "Re-inject native Node modules (#4970)"

This reverts commit ef55e89.

@mjesun mjesun referenced this pull request Jan 2, 2018

Closed

-i has memory issue? #5157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment