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

jsdom dropped support for Node; node/iojs smoke test needs modification #2266

Closed
timmywil opened this Issue May 6, 2015 · 17 comments

Comments

Projects
None yet
5 participants
@timmywil
Member

timmywil commented May 6, 2015

We use jsdom in our Node smoke tests. Unfortunately, the old version doesn't install on iojs and the new one doesn't support Node. We'll have to use something else.

@timmywil timmywil self-assigned this May 6, 2015

@timmywil timmywil added this to the 3.0.0 milestone May 6, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

Actually, another solution would be to install different versions of jsdom using the npm module before running the tests.

Member

timmywil commented May 6, 2015

Actually, another solution would be to install different versions of jsdom using the npm module before running the tests.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

That would be the quicker path to success, but does anyone have suggestions for alternatives to jsdom? My quick searches haven't really turned up anything promising.

Member

timmywil commented May 6, 2015

That would be the quicker path to success, but does anyone have suggestions for alternatives to jsdom? My quick searches haven't really turned up anything promising.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

:/ I feared it would become an issue. jsdom 3.1.2 works just fine with io.js 1.8.1 but breaks with the newest major 2.0.0 release.

Installing it by ourselves unfortunately means skipping package.json but I don't see a better option.

I don't think anything else comes anywhere near close to jsdom in DOM support. @denis-sokolov, do you know about something?

@domenic is there anything else we could do here except to install different jsdom versions based on if we're on Node.js or io.js?

Member

mgol commented May 6, 2015

:/ I feared it would become an issue. jsdom 3.1.2 works just fine with io.js 1.8.1 but breaks with the newest major 2.0.0 release.

Installing it by ourselves unfortunately means skipping package.json but I don't see a better option.

I don't think anything else comes anywhere near close to jsdom in DOM support. @denis-sokolov, do you know about something?

@domenic is there anything else we could do here except to install different jsdom versions based on if we're on Node.js or io.js?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

Using newest jsdom on io.js has the advantage of being easier for Windows users as it doesn't require compilation done by contextify; some users reported to me it's harder for them to do npm install in the jQuery repo because of that.

Member

mgol commented May 6, 2015

Using newest jsdom on io.js has the advantage of being easier for Windows users as it doesn't require compilation done by contextify; some users reported to me it's harder for them to do npm install in the jQuery repo because of that.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 6, 2015

If you get contextify patched to support io.js 2.0, then jsdom 3.x should continue to work everywhere. Someone has to do the work though.

domenic commented May 6, 2015

If you get contextify patched to support io.js 2.0, then jsdom 3.x should continue to work everywhere. Someone has to do the work though.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

contextify was already patched. We could install the latest version of contextify top-level so jsdom uses that, but that seems like a short-term solution.

Member

timmywil commented May 6, 2015

contextify was already patched. We could install the latest version of contextify top-level so jsdom uses that, but that seems like a short-term solution.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

contextify was already patched. We could install the latest version of contextify top-level so jsdom uses that, but that seems like a short-term solution.

That's not needed. jsdom 3.1.2 depends on contextify >= 0.1.9 < 0.2.0 so it will catch a newer vesion if it doesn't bump the minor. The newest contextify is 0.1.13, though, so even if contextify was patched, it hasn't been released yet.

In that case it seems it's best to wait.

Member

mgol commented May 6, 2015

contextify was already patched. We could install the latest version of contextify top-level so jsdom uses that, but that seems like a short-term solution.

That's not needed. jsdom 3.1.2 depends on contextify >= 0.1.9 < 0.2.0 so it will catch a newer vesion if it doesn't bump the minor. The newest contextify is 0.1.13, though, so even if contextify was patched, it hasn't been released yet.

In that case it seems it's best to wait.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

Actually, I think I misunderstood the patch for this issue.

Member

timmywil commented May 6, 2015

Actually, I think I misunderstood the patch for this issue.

@mgol

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov May 6, 2015

I am unaware of anything similar to jsdom.

I think you can use separate versions of jsdom on different platforms and switch them programatically without much problem using version shims and optional dependencies:

var jsdom;
try { jsdom = require('jsdom'); }
catch (e) { jsdom = require('jsdom3'); }

Here jsdom3 is a shim module, see express3 for an example (it's been semi-blessed in npm issue 5499), and both are optional dependencies to handle installation failures on wrong platforms:

"optionalDependencies": {
  "jsdom": "^4.0.0",
  "jsdom3": "^3.0.0"
}

denis-sokolov commented May 6, 2015

I am unaware of anything similar to jsdom.

I think you can use separate versions of jsdom on different platforms and switch them programatically without much problem using version shims and optional dependencies:

var jsdom;
try { jsdom = require('jsdom'); }
catch (e) { jsdom = require('jsdom3'); }

Here jsdom3 is a shim module, see express3 for an example (it's been semi-blessed in npm issue 5499), and both are optional dependencies to handle installation failures on wrong platforms:

"optionalDependencies": {
  "jsdom": "^4.0.0",
  "jsdom3": "^3.0.0"
}
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

@denis-sokolov This solution would make the installation of jsdom3 fail in io.js causing confusion amongst users, especially that the way npm currently informs about a failure in installing an optional dependency is far from ideal.

How about using a special field:

"jsdomVersions": {
  "node": "3.1.2",
  "iojs": "5.3.0"
}

and a postinstall hook that would check if a proper version from require('./package.json').jsdomVersions is already installed and would install it if it's not?

Member

mgol commented May 6, 2015

@denis-sokolov This solution would make the installation of jsdom3 fail in io.js causing confusion amongst users, especially that the way npm currently informs about a failure in installing an optional dependency is far from ideal.

How about using a special field:

"jsdomVersions": {
  "node": "3.1.2",
  "iojs": "5.3.0"
}

and a postinstall hook that would check if a proper version from require('./package.json').jsdomVersions is already installed and would install it if it's not?

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov May 6, 2015

If you dislike the way npm informs about failing to install optional dependencies, I suggest you comment on the issue with npm. After all, our use case for jsdom fits the description of optional dependencies very well.
Besides, jsdom is a development dependency for jQuery, thus "users" who would see the warning are jQuery developers, who are expected to be rather informed, and will likely run npm test regardless.

I find that work with postinstall is a lot more manual steps that can fail for no major benefit.
I would choose to avoid the hook, but it is certainly a solution that will probably work.

denis-sokolov commented May 6, 2015

If you dislike the way npm informs about failing to install optional dependencies, I suggest you comment on the issue with npm. After all, our use case for jsdom fits the description of optional dependencies very well.
Besides, jsdom is a development dependency for jQuery, thus "users" who would see the warning are jQuery developers, who are expected to be rather informed, and will likely run npm test regardless.

I find that work with postinstall is a lot more manual steps that can fail for no major benefit.
I would choose to avoid the hook, but it is certainly a solution that will probably work.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 6, 2015

Member

If you dislike the way npm informs about failing to install optional dependencies

One issue is the misleading message, another is trying to install the dependency, compile it & fail when we know beforehand it will, indeed, fail. This seems wasteful & slow, especially that it will happen on every single npm install, even if you invoke it twice in a row.

Member

mgol commented May 6, 2015

If you dislike the way npm informs about failing to install optional dependencies

One issue is the misleading message, another is trying to install the dependency, compile it & fail when we know beforehand it will, indeed, fail. This seems wasteful & slow, especially that it will happen on every single npm install, even if you invoke it twice in a row.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

check if a proper version from require('./package.json').jsdomVersions is already installed and would install it if it's not

This is pretty much what I was thinking, but since we only need jsdom for the node smoke tests, I was thinking we'd do this check and install only when those tests were run. Otherwise, it's not needed (like for copy PRs).

Member

timmywil commented May 6, 2015

check if a proper version from require('./package.json').jsdomVersions is already installed and would install it if it's not

This is pretty much what I was thinking, but since we only need jsdom for the node smoke tests, I was thinking we'd do this check and install only when those tests were run. Otherwise, it's not needed (like for copy PRs).

@denis-sokolov

This comment has been minimized.

Show comment
Hide comment
@denis-sokolov

denis-sokolov May 6, 2015

One issue is the misleading message, another is trying to install the dependency, compile it & fail when we know beforehand it will, indeed, fail. This seems wasteful & slow, especially that it will happen on every single npm install, even if you invoke it twice in a row.

That is a meaningful concern, although I think it could be addressed by setting correct engine requirements in the jsdom package to let npm know right off the bat about the incompatibilities.

This is pretty much what I was thinking, but since we only need jsdom for the node smoke tests, I was thinking we'd do this check and install only when those tests were run. Otherwise, it's not needed (like for copy PRs).

This sounds best. It can be achieved as simply as (npm install jsdom@3 || npm install jsdom) && run-smoke-tests.

denis-sokolov commented May 6, 2015

One issue is the misleading message, another is trying to install the dependency, compile it & fail when we know beforehand it will, indeed, fail. This seems wasteful & slow, especially that it will happen on every single npm install, even if you invoke it twice in a row.

That is a meaningful concern, although I think it could be addressed by setting correct engine requirements in the jsdom package to let npm know right off the bat about the incompatibilities.

This is pretty much what I was thinking, but since we only need jsdom for the node smoke tests, I was thinking we'd do this check and install only when those tests were run. Otherwise, it's not needed (like for copy PRs).

This sounds best. It can be achieved as simply as (npm install jsdom@3 || npm install jsdom) && run-smoke-tests.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 6, 2015

Member

PR is up.

Member

timmywil commented May 6, 2015

PR is up.

timmywil added a commit to timmywil/jquery that referenced this issue May 6, 2015

timmywil added a commit to timmywil/jquery that referenced this issue May 7, 2015

timmywil added a commit to timmywil/jquery that referenced this issue May 7, 2015

@timmywil timmywil closed this in #2272 May 8, 2015

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 9, 2015

Member

contextify 1.1.14 was released & jsdom 3.1.2 now works fine on io.js 2.0.1. IMO this solution is still better, though as we'll be also testing agains current jsdom & it'll be easier for people installing on io.js as no compilation step is needed.

Member

mgol commented May 9, 2015

contextify 1.1.14 was released & jsdom 3.1.2 now works fine on io.js 2.0.1. IMO this solution is still better, though as we'll be also testing agains current jsdom & it'll be easier for people installing on io.js as no compilation step is needed.

@mgol mgol removed the Needs review label Jul 31, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

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