busterjs/buster

Replace tmpvar/jsdom with cheeriojs/cheerio?

mroderick opened this issue · 6 comments

mroderick commented

Recently, contextify has started failing to build on node@0.6 on both Linux and OSX.

The only dependency on contextify that I can find Buster modules, is in tmpvar/jsdom, which only supports iojs since v4.0, so we won't be able to upgrade ourselves out of this problem or any future problems with jsdom. Even if/when node and iojs merges, it's clear that jsdom project is not interested in maintaining compatibility with legacy node versions (which is a valid choice).

With that in mind, I think it is too much effort to get contextify building on node@0.6, only to be stuck with a jsdom dependency that we can no longer upgrade. I've looked around, and while there are forks of jsdom for node, none seem to be that well maintained.

Also: jsdom doesn't seem to work well in node@0.12, as you can see in my PR to run tests in more node versions

Would you consider merging a pull request that replaces the use of tmpvar/jsdom with cheeriojs/cheerio?

Also, why are there two html reporters? (I'd like to avoid duplication of effort, if I can)

dwittner commented

@mroderick, if cheeriojs/cheerio fits all our needs, there is nothing against it. Does cheeriojs/cheerio needs node-gyp for installation? If not, this would be an advantage too.

There are two html reporters, because html2 is a new, stylish version created by @cjohansen. I am not sure, but i guess the html reporter will be thrown away or replaced by html2 reporter some day.

dominykas commented

cheerio won't help in buster-test - the way jsdom is used, is to construct a full browser-like environment, and then regular DOM functions are being used to build the reporter output. cheerio only provides a jquery-like API to deal with HTML. It might just be enough to build the output, but it would also mean that a compatible module would be needed on the browser side and I'd really hate to add jquery itself as a dependency.

Investigating alternatives...

dominykas commented

Some alternatives I'm thinking about:

  • Remove jsdom from optionalDependencies and be done with it. There's no way to ensure inside package.json the correct version for all nodes - only <=v3 works in 0.10/0.12, only >=v4 works in 4/5. Since it's only being used in html/html2 reporters on node side, it should be safe to not even bother installing it - the end users can install it themselves if they really really want to use these reporters in node side, and not in the browser. I can configure Travis easily to run tests with a correct version. In essence, we're already doing this, since the install fails on node v4+.
  • Install jsdom@latest and also jsdom-compat side by side, pick the correct one based on environment (I'm kind of reluctant to do this, as buster's installation is already taking ages... but this would solve the problem of ensuring there's at least one valid jsdom installed)
  • Use React's virtual-dom / vdom-virtualize (not sure about the effort involved...)
mroderick commented

I think we should just get rid of it. That will benefit most users

dominykas commented

Wait, actually, jsdom@3.x series seem to work in all nodes, so that could be a stopgap solution, although I'm in favour of just removing it to speed up install - not sure it really brings value being there.

dominykas commented

A little bit late to the party, contextify actually released a 0.1.15 some 10 days ago - which luckily allowed me to upgrade jsdom to v3 in buster-test, which I will release as soon as all Travis builds pass (something odd with npm install is breaking them occasionally...) - so that solves the immediate problem. As for the future - we'll see :)

Upd 1: released buster-test@0.7.14 which should now work
Upd 2: have a working travis build for both - jsdom 3 (compat) and 7 (latest): https://travis-ci.org/busterjs/buster-test/builds/92694238
Upd 3: this is actually not a bad way of doing things: https://github.com/busterjs/buster-test/pull/32/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R73 - if someone is on pre-LTS and they are using html reporter on node side, then they can install jsdom@3 locally in their project and npm will be content with it, i.e. jsdom@7 will not be installed and will not be used