[io.js] Update jsdom and get jest working on io.js #374
Conversation
ide
force-pushed
the
iojs
branch
2 times, most recently
from
7ca1234
to
c245dca
Compare
|
@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. |
|
@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: 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. |
|
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 |
|
👍 - could really use some of the stuff jsdom includes with a newer version, things like |
| @@ -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; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
jest should also be able to remove the 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() { | |||
| }; | |||
|
Things that should now be removable:
|
|
Also, just for the record
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 |
|
Thanks for the comments @Sebmaster and @domenic. I got With With |
|
Please do not use underscore-prefixed variables. We can and do change them in patch versions. |
|
@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", |
There was a problem hiding this comment.
Would be nice having this merged to upstream, and to use the new version of CoffeeScript.
There was a problem hiding this comment.
Here's the PR: jashkenas/coffeescript#3985
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 @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? |
|
@domenic The filename is for better stack traces that are similar to the one you get if you I think you're right that |
|
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 |
|
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. |
|
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, { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :(.
|
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 |
|
I am hopeful once @ide has a few days to look at removing the eval and stringify that will be just as performant. |
|
@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. |
|
OK, I'll try updating to jsdom 3.x and fixing the Node 0.12. And after that we can merge this one. |
|
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 evaluateSomehow(`(__wrapperFunction__ = function(${someVariables}) { ${sourceText} })`)So my uuid/random variable code must have been blowing out JIT caches. |
|
Here are some new results. I used
So I'm thinking that we should commit this with the Master on Node 0.10:
|
|
@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. |
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 ```
|
@ide, thanks for the detailed analysis. I see you make it configurable via the Mind keeping this PR for a bit? -- we'll need to prepare for supporting @domenic, in the view of io.js and Node projects rejoining, does it still make sense to explicitly disallow Node for |
|
@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): |
|
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. |
|
OK, sounds good guys, I'll merge it later on today to |
|
Thanks @DmitrySoshnikov! |
|
@ide, do you know whether github supports yet changing target branch of a PR (I just created |
|
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. |
|
Thanks for merging @DmitrySoshnikov. This should make it easy for people to test the new code by npm installing |
|
Yep, thanks, we'll follow up with the cleanup which @domenic mentioned. Also, if people will adopt io.js well, I'll consider merging |
|
working as expected, thanks. |
|
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 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.
--harmonyflag from the TestRunner for the same reason (trivia: i added that line once upon a time, let me delete it)runwith io.js's built-invmmodulenextTickimplementation, which io.js itself relies onnode_modules-- this fixes an issue I was getting with graceful-fs trying to require smalloc.instanceof Array. Working on getting that merged upstream.Fixes #267
Test Plan: