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

[io.js] Update jsdom and get jest working on io.js #374

Closed
wants to merge 2 commits into from

Conversation

ide
Copy link
Contributor

@ide ide commented May 21, 2015

This gets jest working on io.js 1.8.x and 2.0.x. io.js is reconciling with the Node Foundation so users who are still on Node are on the upgrade path that converges with io.js anyway. What this means is the jest version probably should be bumped to 0.5.0, where 0.4.x is a maintenance branch for legacy Node (typically with semver, a major version bump is required but 0.x.x releases are considered a free-for-all).

This diff is near the minimal amount of work to bring jest up to date and pass all of its tests.

  • Upgraded npm dependencies. The major update is jsdom from 0.10.3 to 5.4.3. This lets us drop the indirect contextify dependency.
  • Removed harmonize, since io.js uses modern V8 and enables stable ES6 features by default
  • Removed the --harmony flag from the TestRunner for the same reason (trivia: i added that line once upon a time, let me delete it)
  • Replaced jsdom's run with io.js's built-in vm module
  • Added support for curried arguments in the fake nextTick implementation, which io.js itself relies on
  • Updated the HasteModuleLoader to handle Node built-in modules before node_modules -- this fixes an issue I was getting with graceful-fs trying to require smalloc.
  • Patched coffeescript to use Array.isArray instead of instanceof Array. Working on getting that merged upstream.

Fixes #267

Test Plan:

nvm install iojs-1
rm -fr node_modules && npm install

nvm install iojs-2
rm -fr node_modules && npm install

@ide ide mentioned this pull request May 21, 2015
@ide ide force-pushed the iojs branch 2 times, most recently from 7ca1234 to c245dca Compare May 21, 2015 03:49
@DmitrySoshnikov
Copy link
Contributor

@ide, this is something really great! Does the update fix automatically the node v.0.12 as well (the contextify issue was one of the main there)? I'm on PTO at the moment, but I'll get back to this PR for detailed review in a week. And we can discuss in detail the further plans on supporting separately jest 0.4 and 0.5.

@ide
Copy link
Contributor Author

ide commented May 23, 2015

@DmitrySoshnikov I'm glad that there is interest in this PR.

Unfortunately it does not work with Node 0.12 since jsdom has moved past it. Running the tests gives me:

SyntaxError: /Users/ide/jest/node_modules/jsdom/lib/jsdom.js:3
`jsdom 4.x onward only works on io.js, not Node.js™: https://github.com/tmpvar
^

The syntax error is there because jsdom uses a template literal in order to prevent it from running on older versions of V8. Even with the --harmony flag the tests do not run on Node 0.12.

If Facebook engineers can commit to supporting Node 0.12 and io.js/Node.next then it could make sense to have a branch for each platform. Otherwise, io.js is where most of the momentum and talent is at so it probably makes sense to focus supporting io.js until Node and io merge together for Node 3.0.

@ide
Copy link
Contributor Author

ide commented May 23, 2015

With some small updates, all of the React Native JS tests now pass on io.js 1.8.1 and 2.0.2: facebook/react-native@master...ide:iojs-tests

@brianium
Copy link

👍 - could really use some of the stuff jsdom includes with a newer version, things like lastElementChild and firstElementChild

@@ -29,7 +31,8 @@ function JSDomEnvironment(config) {
// use it (depending on the context -- such as TestRunner.js when operating as
// a workerpool parent), this is the best way to ensure we only spend time
// require()ing this when necessary.
this.global = require('./lib/jsdom-compat').jsdom().parentWindow;
this.global = require('./lib/jsdom-compat').jsdom().defaultView;
this.global.window = this.global;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks problematic. In jsdom we had problems with making sure this === window && document.window === window && window.window === window with some similiar constructs, are you sure that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's broken, this is undefined under the new vm setup. I don't have time to look into this right now but maybe someone can figure out how to set this to the correct value in the sandboxed context.

@Sebmaster
Copy link
Contributor

jest should also be able to remove the Image constructor when updating jsdom, since it has been added to jsdom directly.

It should also be possible to remove the majority of the API pass-throughs, since they've been added to V8 natively.

@@ -105,7 +107,10 @@ JSDomEnvironment.prototype.dispose = function() {
};
Copy link

Choose a reason for hiding this comment

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

Hopefully dispose() can be reinstated

@domenic
Copy link

domenic commented May 29, 2015

Things that should now be removable:

@domenic
Copy link

domenic commented May 29, 2015

Also, just for the record

The syntax error is there because jsdom uses a template literal in order to prevent it from running on older versions of V8.

In particular we did this because we use template strings extensively, and people were filing bugs saying "you have a syntax error; replace your ``` with '". Since changing the first template string encountered to be that particular template string, the number of such spurious bugs has dropped dramatically :)

@ide
Copy link
Contributor Author

ide commented Jun 2, 2015

Thanks for the comments @Sebmaster and @domenic. I got global === this working, both by using document._ownerDocument._global and a <script> tag. I ended up going with the vm call because my implementation using the <script> tag was 10x slower on some tests and more complicated. This is the gist if anyone would like to take a look: https://gist.github.com/ide/77d6e4d907083f23143e. There's an extra stringify and eval call in order to capture the result of the evaluated script.

With vm.runInContext:

$ node bin/jest.js
Using Jest CLI v0.4.5
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.022s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.532s)
 PASS  __tests__/TestRunner-test.js (0.558s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.132s)
 PASS  lib/__tests__/FakeTimers-test.js (0.109s)
 PASS  lib/__tests__/moduleMocker-test.js (0.029s)
 PASS  __tests__/TestRunner-fs-test.js (1.146s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.214s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.047s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (1.362s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (1.563s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (1.541s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (2.282s)
Waiting on 2 tests..._writableState.buffer is deprecated. Use _writableState.getBuffer() instead.
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (3.075s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (3.408s)
15 tests passed (15 total)
Run time: 4.818s

With <script>:

$ node bin/jest.js
Using Jest CLI v0.4.5
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.035s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.792s)
 PASS  __tests__/TestRunner-test.js (1.258s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.15s)
 PASS  lib/__tests__/FakeTimers-test.js (0.223s)
 PASS  lib/__tests__/moduleMocker-test.js (0.025s)
 PASS  __tests__/TestRunner-fs-test.js (2.149s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (2.191s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.074s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.652s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (3.638s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (5.392s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (8.758s)
Waiting on 2 tests..._writableState.buffer is deprecated. Use _writableState.getBuffer() instead.
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (29.202s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (43.083s)
15 tests passed (15 total)
Run time: 44.466s

@domenic
Copy link

domenic commented Jun 2, 2015

Please do not use underscore-prefixed variables. We can and do change them in patch versions.

@domenic
Copy link

domenic commented Jun 2, 2015

@ide looking at the gist, isn't it possible to avoid the stringify and eval (and maybe the trip through the resource loader)? I am not sure all of the contexts in which runSourceText is used, but I think the main place is via runContentWithLocalBindings, which always returns source text for a wrapper function. That would I think allow something like

  var tempVariable = '_' + uuid().replace(/-/g, '_');
  this._scriptToServe = 'var ' + tempVariable + ' = ' + sourceText + '()';

It's not 100% clear to me why you need the filename either (which is why the round trip through the resource loader is necessary), but maybe it is important for stack traces or similar.

"jasmine-only": "0.1.0",
"jasmine-pit": "~2.0.0",
"jsdom": "~0.10.3",
"coffee-script": "ide/coffeescript#array-check",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice having this merged to upstream, and to use the new version of CoffeeScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DmitrySoshnikov
Copy link
Contributor

<script> tag was 10x slower

Performance is currently one of the Jest's issues which invest the most resources, so don't think we can degrade it this way with the <script>.

@domenic, would be great to have some workaround if the global object is a private data (any suggestions?). Or, could we just expose getter for the global object for public API?

@ide
Copy link
Contributor Author

ide commented Jun 3, 2015

@domenic The filename is for better stack traces that are similar to the one you get if you require() a module that throws an error during initialization. I believe the intent is to mimic require's implementation but run the code in jsdom's context. In some ways, using vm.runInContext is a nice solution because it is so similar to Node's implementation of require, which also uses the vm module. If you and the other jsdom maintainers are open to exposing the jsdom context I think that could work well here.

I think you're right that runSourceText is mostly used with runContentWithLocalBindings and we'd be able to remove the stringify + eval since we know the evaluated code is always a function expression. It will be a few days before I can look into this, if the <script> tag is really the way we go.

@domenic
Copy link

domenic commented Jun 3, 2015

I mean, we definitely don't want to expose the actual global object, as that is super-unlike browsers. If we need some other API (like runScriptWithoutScriptTag or something) we could consider that. But I'd really hope we can just make this work in the normal way, that you would with a browser, i.e. a <script> tag.

@ide
Copy link
Contributor Author

ide commented Jun 3, 2015

Here's a quick proposal for exposing the jsdom context:

var context = vm.createContext({});
jsdom.jsdom(markup, {
  vmContext: context  // jsdom uses this context instead of creating its own
});

Not sure how easy this is to implement but shouldn't compromise the jsdom API.

@domenic
Copy link

domenic commented Jun 3, 2015

No, we want to erase the distinction between global object and global proxy as much as possible, both through future changes to jsdom and future changes to io.js. Baking that distinction in to the API is a non-starter.

// TODO: Stop using the private API and instead configure jsdom with a
// resource loader and insert <script src="${filename}"> in the document. The
// reason we use vm for now is because the script element technique is slow.
return vm.runInContext(sourceText, this.document._ownerDocument._global, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, would be nice having it exposed as public API (e.g. getter only for the global object, or something, @Sebmaster, @domenic). Also, this implementation details make it less clear in the code (for those who will be dealing with it later) why we use this.document._ownerDocument._global, but also have e.g. this.global (which is this.document.defaultView). So worth adding a comment about it as well.

Copy link

Choose a reason for hiding this comment

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

Yeah we're discussing this in the rest of the PR. This particular technique is very very bad and will break easily in patch jsdom versions. Never access underscore-prefixed properties :(.

@DmitrySoshnikov
Copy link
Contributor

I see, thanks for the info guys. Yeah, we'll need something that wouldn't degrade the performance, since again it's one of the main Jest's issues at the moment. The runScriptWithoutScriptTag could be good solution.

@domenic
Copy link

domenic commented Jun 3, 2015

I am hopeful once @ide has a few days to look at removing the eval and stringify that will be just as performant.

@ide
Copy link
Contributor Author

ide commented Jun 16, 2015

@DmitrySoshnikov we could try jsdom 3.x with this patch and it might work on Node 0.12 without much extra work. I believe that Node 0.12 and io.js 1.0 are more similar to each other than Node 0.10 and Node 0.12.

@DmitrySoshnikov
Copy link
Contributor

OK, I'll try updating to jsdom 3.x and fixing the Node 0.12. And after that we can merge this one.

@ide
Copy link
Contributor Author

ide commented Jun 17, 2015

I got the tests passing again but found something very strange... the source of the slowness isn't jsdom's script loader (there is some overhead but it's under a second) and I don't think it was the extra eval call either. What I'm seeing is that this is fast:

evaluateSomehow(`(function(${someVariables}) { ${sourceText} })`)

and this is very slow:

evaluateSomehow(`(${randomTempVariable} = function(${someVariables}) { ${sourceText} })`)

I assign to a temp variable when using jsdom's <script> tag so that I can capture the result of defining the function. The good news is that writing to the same variable over and over is fast:

evaluateSomehow(`(__wrapperFunction__ = function(${someVariables}) { ${sourceText} })`)

So my uuid/random variable code must have been blowing out JIT caches.

@ide
Copy link
Contributor Author

ide commented Jun 17, 2015

Here are some new results. I used vm, jsdom <script src>, and jsdom <script> with text content, all all three on io.js outperform master on Node 0.10, so that is a positive outcome! In order of performance on my laptop we have:

technique time notes
vm 2.30 seconds 31% improvement over master
script 2.57 seconds the filename in one stack frame becomes unhelpful like (about:blank:undefined:undefined<script>:10:7)
<script src> 2.80 seconds the filename in one stack frame gets file:// prepended to it
master 3.34 seconds

So I'm thinking that we should commit this with the <script src> technique, which is the slowest but provides a helpful stack trace entry and is still faster than master by 16%. Then later it probably makes sense for jsdom to expose the vm context or provide runScriptWithoutScriptElement which would let jsdom run about 18% faster compared to <script src>.


Master on Node 0.10:

$ node bin/jest.js 
Using Jest CLI v0.4.13
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.014s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.332s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (0.675s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (0.761s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.135s)
 PASS  lib/__tests__/FakeTimers-test.js (0.095s)
 PASS  lib/__tests__/moduleMocker-test.js (0.025s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.14s)
 PASS  __tests__/TestRunner-test.js (1.221s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (1.266s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.778s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (1.394s)
 PASS  __tests__/TestRunner-fs-test.js (1.834s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (2.353s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (2.725s)
15 tests passed (15 total)
Run time: 3.344s

vm.runInContext on io.js 2.3.0:

$ node bin/jest.js 
Using Jest CLI v0.4.13
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.018s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.25s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (0.315s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.1s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (0.451s)
 PASS  lib/__tests__/moduleMocker-test.js (0.022s)
 PASS  __tests__/TestRunner-test.js (0.562s)
 PASS  lib/__tests__/FakeTimers-test.js (0.094s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (0.615s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.054s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.235s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (0.641s)
 PASS  __tests__/TestRunner-fs-test.js (1.072s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (1.156s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (1.207s)
15 tests passed (15 total)
Run time: 2.298s

<script src> on io.js 2.3.0:

$ node bin/jest.js 
Using Jest CLI v0.4.13
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.033s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (0.448s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.355s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.148s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (0.761s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (0.773s)
 PASS  lib/__tests__/moduleMocker-test.js (0.039s)
 PASS  lib/__tests__/FakeTimers-test.js (0.186s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.088s)
 PASS  __tests__/TestRunner-test.js (1.002s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (0.868s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.565s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (1.614s)
 PASS  __tests__/TestRunner-fs-test.js (1.712s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (1.714s)
15 tests passed (15 total)
Run time: 2.798s

<script>${source}</script> on io.js 2.3.0:

$ node bin/jest.js 
Using Jest CLI v0.4.13
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-currentTestPath-test.js (0.025s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-genMockFromModule-test.js (0.336s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-getTestEnvData-test.js (0.412s)
 PASS  jasmineTestRunner/__tests__/JasmineReporter-test.js (0.12s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-NODE_PATH-test.js (0.653s)
 PASS  lib/__tests__/moduleMocker-test.js (0.027s)
 PASS  lib/__tests__/FakeTimers-test.js (0.117s)
 PASS  lib/__tests__/utils-pathNormalize-test.js (0.091s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-hasDependency-test.js (0.875s)
 PASS  __tests__/TestRunner-test.js (0.94s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModuleOrMock-test.js (0.762s)
 PASS  __tests__/TestRunner-fs-test.js (1.304s)
 PASS  lib/__tests__/utils-normalizeConfig-test.js (0.576s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireMock-test.js (1.432s)
 PASS  HasteModuleLoader/__tests__/HasteModuleLoader-requireModule-test.js (1.503s)
15 tests passed (15 total)
Run time: 2.572s

@ide
Copy link
Contributor Author

ide commented Jun 17, 2015

@DmitrySoshnikov you may have better luck but I adjusted this patch a little bit to run it on Node 0.12.4 and it segfaults in V8 somewhere so I'm going to stick to this io.js patch.

PID 95888 received SIGSEGV for address: 0x0
0   segfault-handler.node               0x00000001011f091a _ZL16segfault_handleriP9__siginfoPv + 282
1   libsystem_platform.dylib            0x00007fff8fe68f1a _sigtramp + 26
2   ???                                 0x000000000000000a 0x0 + 10
3   node                                0x000000010050f3cf _ZN4nodeL9EnvGetterEN2v85LocalINS0_6StringEEERKNS0_20PropertyCallbackInfoINS0_5ValueEEE + 60
4   node                                0x000000010013e892 _ZN2v88internal25PropertyCallbackArguments4CallEPFvNS_5LocalINS_6StringEEERKNS_20PropertyCallbackInfoINS_5ValueEEEES4_ + 146
5   node                                0x000000010037fbdf _ZN2v88internal8JSObject26GetPropertyWithInterceptorENS0_6HandleIS1_EENS2_INS0_6ObjectEEENS2_INS0_4NameEEE + 351
6   node                                0x000000010037f803 _ZN2v88internal6Object11GetPropertyEPNS0_14LookupIteratorE + 195
7   node                                0x000000010033cc78 _ZN2v88internal6LoadIC4LoadENS0_6HandleINS0_6ObjectEEENS2_INS0_4NameEEE + 1176
8   node                                0x0000000100342a96 _ZN2v88internal11LoadIC_MissEiPPNS0_6ObjectEPNS0_7IsolateE + 134
9   ???                                 0x0000175e9ee060a2 0x0 + 25695159869602
10  ???                                 0x0000175e9f2aee1d 0x0 + 25695164755485

ide added 2 commits June 22, 2015 20:55
This gets jest working on io.js 1.8.x and 2.0.x. io.js is reconciling with the Node Foundation so users who are still on Node are on the upgrade path that converges with io.js anyway. What this means is the jest version probably should be bumped to 0.5.0, where 0.4.x is a maintenance branch for legacy Node (typically with semver, a major version bump is required but 0.x.x releases are considered a free-for-all).

This diff is near the minimal amount of work to bring jest up to date and pass all of its tests.

 - Upgraded npm dependencies. The major update is jsdom from 0.10.3 to 5.4.3. This lets us drop the indirect contextify dependency.
 - Removed harmonize, since io.js uses modern V8 and enables stable ES6 features by default
 - Removed the `--harmony` flag from the TestRunner for the same reason (trivia: i added that line once upon a time, let me delete it)
 - Replaced jsdom's `run` with io.js's built-in `vm` module
 - Added support for curried arguments in the fake `nextTick` implementation, which io.js itself relies on
 - Updated the HasteModuleLoader to handle Node built-in modules before `node_modules` -- this fixes an issue I was getting with graceful-fs trying to require smalloc.
 - Patched coffeescript to use Array.isArray instead of `instanceof Array`. Working on getting that merged upstream.

Also mocked out JSDomEnvironment under __mocks__ instead of doing it in each test. Each test used to implement its own JSDomEnvironment. This diff removes the copy-pasted code and introduces a shared mock under __mocks__.

Test Plan:
```
nvm install iojs-1
rm -fr node_modules && npm install

nvm install iojs-2
rm -fr node_modules && npm install
```
@DmitrySoshnikov
Copy link
Contributor

@ide, thanks for the detailed analysis. I see you make it configurable via the USE_JSDOM_EVAL variable (although it's local to a file, and cannot be passed as an option or something). At FB on www we try to win every ms of performance now, so maybe we'll use still the way with vm there.

Mind keeping this PR for a bit? -- we'll need to prepare for supporting 0.5.x and legacy 0.4.x in parallel, and announcing that Jest will be working only with io.js. Also, I still haven't tried running it on 0.12.

@domenic, in the view of io.js and Node projects rejoining, does it still make sense to explicitly disallow Node for jsdom? Although, as official documentation says "io.js releases will continue in parallel". This "in parallel" makes me worried, since if we're gonna work only with io.js, there will be no way to return to Node, if node 0.13, 0.14, etc will appear, and devs will stuck forever with io.js, and probably will have to maintain different versions of node.

@ide
Copy link
Contributor Author

ide commented Jun 23, 2015

@DmitrySoshnikov could you create a "0.5.x" branch and merge this PR to that branch? Since this diff touches a lot of files there are sometimes rebase conflicts that I am hoping to avoid.

Regarding Node and io.js, I believe the main difference that affects jest is the version of V8 they use. In the new Node repository they are using V8 4.2 which matches io.js so supporting io.js today should make it easier to support the future version of Node when the projects converge.

Also that page says, "While the project streams are being converged, io.js releases will continue in parallel," which implies that once the two projects are done converging, there will be only project. From what I've read, the converged project will be based on io.js's codebase but will be called "Node". So if jest works only with io.js, it's true that jest maybe won't work with Node 0.13/0.14 but it will work with the converged version of Node (for example Node 3.0, and io.js will stop releasing major updates).

This diagram shows the plan for io.js + Node (except that io.js is already at 2.x, so the converged version would be Node.js 3.0):

                        now
       (io.js)    v1.6   :  v1.7    v1.x
          |         |    :    |       |
 v0.10.x  /--------------:-----------------\   Node.js 2.0
____|____/               :                  \______|_____
         \               :                  /
          \--------------:-----------------/
          |         |    :     |       |
       (node.js) v0.12.x :  v0.13.x  v0.14.x

@domenic
Copy link

domenic commented Jun 23, 2015

Indeed, once a new Node release is made based on the io.js codebase, jsdom will work with that. But before then, node.js is still not a suitable substrate.

I doubt any 0.13 or 0.14 releases will ever happen. And I think io.js 3.0 will be released before a "converged" (renamed) release. My guess is that in lieu of io.js 5.0 we will see Node.js jump from 0.12 to 5.0. But until then jsdom stays io.js only.

@DmitrySoshnikov
Copy link
Contributor

OK, sounds good guys, I'll merge it later on today to 0.5.x.

@ide
Copy link
Contributor Author

ide commented Jun 23, 2015

Thanks @DmitrySoshnikov!

@DmitrySoshnikov
Copy link
Contributor

@ide, do you know whether github supports yet changing target branch of a PR (I just created 0.5.x branch)? If not, I'll merge it manually from command line.

@DmitrySoshnikov
Copy link
Contributor

OK, merged manually to https://github.com/facebook/jest/tree/0.5.x (I think we can close the PR manually to now). @ide, thanks a lot for this PR! @domenic, thanks for the info on jsdom and the things.

@ide
Copy link
Contributor Author

ide commented Jun 24, 2015

Thanks for merging @DmitrySoshnikov. This should make it easy for people to test the new code by npm installing facebook/jest#0.5.x. There should also be some nice opportunities to clean up code on that branch since jsdom is now up-to-date. Closing this PR now.

@ide ide closed this Jun 24, 2015
@DmitrySoshnikov
Copy link
Contributor

Yep, thanks, we'll follow up with the cleanup which @domenic mentioned. Also, if people will adopt io.js well, I'll consider merging 0.5.x to master soon (just tested it, it is faster), and 0.4.x will stay legacy of Node 0.10.

@moonith
Copy link

moonith commented Jun 25, 2015

working as expected, thanks.

@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 14, 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.

Update to JSDOM 4.0
7 participants