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

Make it possible to run jsdom in a worker #711

Closed
lawnsea opened this issue Jan 1, 2014 · 19 comments · Fixed by #849
Closed

Make it possible to run jsdom in a worker #711

lawnsea opened this issue Jan 1, 2014 · 19 comments · Fixed by #849

Comments

@lawnsea
Copy link
Contributor

lawnsea commented Jan 1, 2014

For my research project Treehouse, I forked jsdom and got it to run in a web worker. This was two years ago. I'm interested in doing this again, but in a way that is maintainable, rather than just a proof-of-concept.

At a high level, this is what was necessary to get jsdom 0.2.4 running in a worker:

  • remove node dependencies that cannot be met in the browser (fs, e.g.)*
  • provide pure-JS implementations of node dependencies where possible (url, e.g.)*
  • use util.inherits instead of assigning to __proto__
  • use Object.defineProperty instead of __defineSetter__/Getter__
  • replace uses of variables named window and document with win and doc**

I also refactored the code to use AMD and build with RequireJS. I don't think this is necessary now (and may not have been necessary then): it should be possible to use the cjsTranslate method to build for the browser.

I would like to keep the diff from my fork to the original as minimal as possible. So, I'd like to upstream those changes that support browser compatibility in a non-breaking way. Would y'all be open to that?

* browserify may help with these
** I'm embarrassed to admit I don't remember why this was necessary

EDIT: I'm aware of this thread and #245

@domenic
Copy link
Member

domenic commented Jan 2, 2014

Wow, this is great.

I think the path these days is to use browserify. So that should eliminate the need to provide pure-JS implementations of the node dependencies, since browserify comes with those; it may also solve the node dependencies that cannot be met. And should obviate the need to use AMD at all; just produce a browserify UMD bundle, which last I heard are worker compatible (or if they're not, that should be an easy PR to get accepted).

The other thing that gives you is the ability to use the browser field in package.json to selectively overwrite modules inside jsdom. That'll be quite useful I imagine, although it could require breaking apart jsdom into smaller modules than exist currently.

use util.inherits instead of assigning to __proto__

I'd prefer not to add the super_ property that util.inherits does, but I'd be totally open to converting to the correct

Subclass.prototype = Object.create(Superclass.prototype, {
  constructor: {
    value: Subclass,
    writable: true,
    configurable: true
  }
});

use Object.defineProperty instead of __defineSetter__/Getter__

Yes please!

replace uses of variables named window and document with win and doc

Hopefully not necessary, but if we can figure it out, I don't see a real problem with it.

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 2, 2014

Ok, great! Thanks for the feedback, @domenic.

I'm going to do the switch to Object.defineProperty and the new inheritance scheme first. Then I'll tackle browserify. Shall I leave this issue open as a tracking bug?

@domenic
Copy link
Member

domenic commented Jan 2, 2014

Yeah sounds good! I'll merge as things come in.

lawnsea added a commit to TreehouseJS/jsdom that referenced this issue Jan 3, 2014
Replace all instances of `__defineGetter__` and `__defineSetter__` with the
respective method from `jsdom/utils`. This is work toward jsdom#711.

For posterity, the vim regex I used is:
`\(\s*\)\([^_]\+\)\.__\([^_]\+\)__(\([^,]\+\),/\1\3(\2, \4,`
This was referenced Jan 3, 2014
domenic pushed a commit that referenced this issue Jan 6, 2014
Replace all instances of `__defineGetter__` and `__defineSetter__` with the
respective method from `jsdom/utils`. This is work toward #711.

For posterity, the vim regex I used is:
`\(\s*\)\([^_]\+\)\.__\([^_]\+\)__(\([^,]\+\),/\1\3(\2, \4,`
@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 20, 2014

Ok, I've surveyed all external dependencies and tried requiring them in a worker via browserify.

The following don't work and will likely never work:

  • contexify: V8 only; there is no browser alternative
  • canvas: depends on cairo; a non-trivial fix would be to implement a JS-only canvas shim backed by a typed array

The following don't work but can likely be fixed:

  • http/http: uses window instead of self. fixed in Use 'global' instead of window. Fixes #36 browserify/http-browserify#37
  • cssstyle: uses fs.readdirSync to read in property files. we'll want to do that at build time instead.
  • request: browserify fails with Error: ENOENT, open 'tls' while resolving "tls" from file /Users/lon/shared/dev/src/treehousejs/jsdom/node_modules/request/node_modules/forever-agent/index.js

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 20, 2014

Ok, it looks like we may be able to use iriscouch/browser-request instead of mikeal/request.

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 24, 2014

@domenic, do you have any preference for how I should accomplish the browserify build? I could just add an entry to scripts in package.json. Or, we could get crazy with grunt, gulp, or whatever.

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 24, 2014

🎆 🍻

I can build a browserify bundle that works with this worker (in Chrome Stable latest):

var jsdom = require('../lib/jsdom');

jsdom.env({
  url: 'http://example.com',
  html: '<p><a class="the-link" href="https://github.com/tmpvar/jsdom">jsdom\'s Homepage</a></p>',
  done: function (errors, window) {
    var link = window.document.querySelector('a.the-link');
    postMessage(link.innerHTML);
  }
});

That was fun. Next step is to get the tests running in a worker and find out how many we fail.

@domenic
Copy link
Member

domenic commented Jan 24, 2014

Woah, nice!

do you have any preference for how I should accomplish the browserify build? I could just add an entry to scripts in package.json. Or, we could get crazy with grunt, gulp, or whatever.

What kind of build is required? I was kind of hoping we'd just use the browser field in package.json to override a bunch of dependencies.

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 25, 2014

Yep! That's how it works. The only real wrinkle is that OS X needs the maximum number of files bumped up.

It's possible we will need something more advanced in the future, but adding a browserify entry to scripts should do the job for now.

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 25, 2014

Ok, I hacked on a copy of test/runner and got it to run the tests. Here's the current status:

  • N/M: N of M tests passed
  • ok: all tests passed
  • fail: the suite fails to run to completion
  • N/A: this suite is commented out in test/runner
"level1/core": ok
"level1/html": ok
"level1/svg": ok
"level2/core": ok
"level2/html": 6/708
"level2/style": 0/15
"level2/extra": ok
"level2/events": ok
"level3/core": N/A
"level3/ls": N/A
"level3/xpath": 0/93
"window/index": ok
"window/history": 0/5
"window/script": 0/10
"window/console": 0/2
"window/frame": fail
"sizzle/index": fail
"jsdom/index": fail
"jsdom/parsing": 0/11
"jsdom/env": 9/25
"jsdom/utils": ok
"jsonp/jsonp": 0/1
"browser/css": 0/1
"browser/index": 30/34

@lawnsea
Copy link
Contributor Author

lawnsea commented Jan 25, 2014

The tests fail for a variety of reasons, most of which I have not diagnosed. Use of fs.readFileSync and http.createServer are two major causes. The former causes the auto-detection magic in jsdom.env to fall over. Another problem is that jsdom.level does dynamic requires, which browserify doesn't support.

@domenic
Copy link
Member

domenic commented Jan 25, 2014

Yep! That's how it works.

Great! In that case I don't think a scripts entry is necessary or desirable. We'll just add the browser field to package.json, and maybe some instructions for building with browserify to the readme if it's harder than just browserify jsdom.

Use of fs.readFileSync

brfs might help here.

@andrew-luhring
Copy link

This reply references the problem:
specifically:
canvas: depends on cairo; a non-trivial fix would be to implement a JS-only canvas shim backed by a typed array

The following don't work and will likely never work:
contexify: V8 only; there is no browser alternative
canvas: depends on cairo; a non-trivial fix would be to implement a JS-only canvas shim backed by a typed array

the exact error I receive is:
request: browserify fails with Error: ENOENT, open 'tls' while resolving "tls" from file /Users/lon/shared/dev/src/treehousejs/jsdom/node_modules/request/node_modules/forever-agent/index.js

and thankfully I see that at least you all know about it.
The question is:

what do I do about it?

@andrew-luhring
Copy link

Actually, the canvas error is from:
filename: 'canvas',
parent: '/node_modules/jsdom/lib/jsdom/level2/html.js' }
Error: module "canvas" not found from "/node_modules/jsdom/lib/jsdom/level2/html.js"

@lawnsea
Copy link
Contributor Author

lawnsea commented Mar 21, 2014

@andrew-luhring You'll need to make a fake canvas module, or remove the require call. I'm afraid you'll have to wait until I have a few hours to make a PR out of my local tree.

@andrew-luhring
Copy link

its all good. where would i put the fake canvas module?
also, thanks for the quick response! that was... really quick!

@chrisabrams
Copy link

@lawnsea +1

@andrew-luhring
Copy link

+2

lawnsea added a commit to TreehouseJS/jsdom that referenced this issue Jul 30, 2014
For jsdom#711, the test runner will execute in a web worker, but test results will be
displayed in the parent page or a shell. So, it is convenient to break the
options parsing, test running, and results display into their own modules for
reuse by other runners.
domenic pushed a commit that referenced this issue Aug 1, 2014
For #711, the test runner will execute in a web worker, but test results will be
displayed in the parent page or a shell. So, it is convenient to break the
options parsing, test running, and results display into their own modules for
reuse by other runners.
@lawnsea
Copy link
Contributor Author

lawnsea commented Aug 2, 2014

It's been a long time coming, but I finally have a PR that adds experimental support for running jsdom browserified in a Web Worker. See #849 for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants