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

[docs] Update "jsdom" guides for v10 and later #921

Merged
merged 1 commit into from
May 18, 2017
Merged

[docs] Update "jsdom" guides for v10 and later #921

merged 1 commit into from
May 18, 2017

Conversation

kenju
Copy link
Contributor

@kenju kenju commented Apr 28, 2017

What

Updated "jsdom" guides which is based on "jsdom" v10 and later API

Why

jsdom has recently updated to v10, but the enzyme's guides is based on "old-api". jsdom says it supports the old api yet, so I did not delete the old code block but instead add a new code block.

The old API is still supported for now

Fixes #942.

/* setup.js */

var { JSDOM } = require('jsdom');
var jsdom = new JSDOM('');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nits chage though. the latter parts is the same, so maybe better way to slim up?

Copy link

Choose a reason for hiding this comment

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

I don't think this is enough, either. With the proposed doc changes, I see the same errors as reported here: jsdom/jsdom#1827.

var { JSDOM } = require('jsdom');
var jsdom = new JSDOM('');

global.document = jsdom;
Copy link

Choose a reason for hiding this comment

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

Assuming the above (var jsdom = new JSDOM('');), this should be:

global.document = jsdom.window.document;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing.

I fixed the doc changes with 35c2f5e

I created a working sample repository with jsdom ^10.1.0.
https://github.com/kenju/enzyme-draftjs-sample/blob/master/test/setup.js

global.window = window;
global.document = window.document;

Object.keys(window).forEach((property) => {
Copy link
Member

Choose a reason for hiding this comment

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

this should use Object.getOwnPropertyDescriptors (or Object.getOwnPropertyNames), not Object.keys, to ensure non-enumerables are grabbed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the advise, that way would be better!

Fixed with: 57dc37a


Object.keys(window).forEach((property) => {
if (typeof global[property] === 'undefined') {
global[property] = window[property];
Copy link
Member

Choose a reason for hiding this comment

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

this should use defineProperty with a property descriptor to maintain nonenumerability and nonwritability when applicable.


function copyProps(src, target) {
const props = Object.getOwnPropertyNames(src)
.filter(prop => typeof target[prop] === 'undefined')
Copy link
Member

Choose a reason for hiding this comment

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

i think you want to keep the ones where it's not undefined, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, actually I meant to filter out ones which are undefined at the target object.

this is equivalent with below code (would you prefer this style?):

  Object.getOwnPropertyNames(src).forEach((prop) => {
    if (typeof target[prop] === 'undefined') { // if not defined yet as the 'target's props
      Object.getOwnPropertyDescriptor(src, prop);
    }
  });

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you're right, thanks for clarifying :-)

ljharb
ljharb previously approved these changes May 11, 2017
@eddies
Copy link

eddies commented May 15, 2017

I meant to create a minimal test case to replicate, but haven't had a chance yet. In the meantime, however, I wanted to report that with this latest version of the documentation, I get XMLHttpRequest is not defined in my project's tests.

@kenju
Copy link
Contributor Author

kenju commented May 16, 2017

@eddies

Does your code premise that XMLHttpRequest exists in global object?

XMLHttpRequest seems to be attached to jsdom.window.
https://github.com/tmpvar/jsdom/search?utf8=%E2%9C%93&q=XMLHttpRequest&type=

Does attaching jsdom.window.XMLHttpRequest to global help for your case like below code?

global.XMLHttpRequest = jsdom.window.XMLHttpRequest;

or,

global.XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest;

@ljharb
Copy link
Member

ljharb commented May 16, 2017

It's a reasonable assumption for browser code to treat all window properties as globals; and for jsdom to be properly simulating a browser, all window properties should be installed on the global object.

@ljharb
Copy link
Member

ljharb commented May 16, 2017

@kenju would you mind rebasing this?

@kenju
Copy link
Contributor Author

kenju commented May 18, 2017

@ljharb Sure I did ->
a693327

@kenju
Copy link
Contributor Author

kenju commented May 18, 2017

@eddies

what is the version of your jsdom? would you try below script out if XMLHttpRequest property can be found. XMLHttpRequest can be found in this sample repository, with jsdom ^v10.1.0:

  const props = Object.getOwnPropertyNames(src)
    .filter(prop => typeof target[prop] === 'undefined')
    .map(prop => {
      if (/XMLHttpRequest/.test(prop)) { // checks if 'XMLHttpRequest'-like props found or not
        console.log(`${prop} found`);
      }
      return Object.getOwnPropertyDescriptor(src, prop);
    });

@ljharb
Copy link
Member

ljharb commented May 18, 2017

@kenju it'll need another rebase; see "This branch is out-of-date with the base branch" below

@kenju
Copy link
Contributor Author

kenju commented May 18, 2017

@ljharb Oops I have missed that, now updated with the latest master branch

@nfcampos nfcampos merged commit f3bbfd3 into enzymejs:master May 18, 2017
@mattnedrich
Copy link

mattnedrich commented Jun 13, 2017

It looks like while this is merged, it has yet to be released - is that correct? The docs still look old - http://airbnb.io/enzyme/docs/guides/jsdom.html.

I ran into an issue getting Enzyme + ReactResponsive to work in a test. Glad I came across this PR -nice work, the updated JSDOM setup fixed the issue I was having.

@ljharb
Copy link
Member

ljharb commented Jun 13, 2017

Yes, that's correct.

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

Successfully merging this pull request may close these issues.

5 participants