Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Prevent error in pretty-format for window in jsdom test env #4750

Merged
merged 7 commits into from
Oct 24, 2017
Merged

Prevent error in pretty-format for window in jsdom test env #4750

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
28a5a16
Prevent error in pretty-format for window in jsdom test env
pedrottimark Oct 23, 2017
0c5e9bb
Delete global
pedrottimark Oct 23, 2017
3fbc2ac
Save file after correcting comments :(
pedrottimark Oct 23, 2017
6c00595
Edit comments after perf measurements
pedrottimark Oct 24, 2017
620e1c0
add to Fixes in changelog
pedrottimark Oct 24, 2017
c00d3e3
Resolve merge conflict in change log
pedrottimark Oct 24, 2017
87d89cf
Simplify with typeof window as suggested by SimenB
pedrottimark Oct 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/pretty-format/src/__tests__/dom_element.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ const toPrettyPrintTo = require('./expect_util').getPrettyPrint([DOMElement]);
const expect: any = global.expect;
expect.extend({toPrettyPrintTo});

describe('pretty-format', () => {
// Test is not related to plugin but is related to jsdom testing environment.
it('prints global window as constructor name alone', () => {
expect(prettyFormat(window)).toEqual('[Window]');
});
});

describe('DOMElement Plugin', () => {
it('supports a single HTML element', () => {
expect(document.createElement('div')).toPrettyPrintTo('<div />');
Expand Down
24 changes: 23 additions & 1 deletion packages/pretty-format/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,26 @@ const errorToString = Error.prototype.toString;
const regExpToString = RegExp.prototype.toString;
const symbolToString = Symbol.prototype.toString;

// Return whether val is equal to global window object.
let noWindow;
let theWindow;
const isWindow = val => {
Copy link
Member

Choose a reason for hiding this comment

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

I still don't think this is necessary. This change passes your test as well:

diff --git i/packages/pretty-format/src/index.js w/packages/pretty-format/src/index.js
index 35a591c9..ba4f91f1 100644
--- i/packages/pretty-format/src/index.js
+++ w/packages/pretty-format/src/index.js
@@ -41,6 +41,8 @@ const errorToString = Error.prototype.toString;
 const regExpToString = RegExp.prototype.toString;
 const symbolToString = Symbol.prototype.toString;
 
+const isWindow = val => typeof window !== 'undefined' && val === window;
+
 const SYMBOL_REGEXP = /^Symbol\((.*)\)(.*)$/;
 const NEWLINE_REGEXP = /\n/gi;
 
@@ -224,7 +226,7 @@ function printComplexValue(
         '}';
   }
 
-  return hitMaxDepth
+  return hitMaxDepth || isWindow(val)
     ? '[' + (val.constructor ? val.constructor.name : 'Object') + ']'
     : (min ? '' : (val.constructor ? val.constructor.name : 'Object') + ' ') +
       '{' +

Copy link
Member

Choose a reason for hiding this comment

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

I suppose you can cache the typeof check, but that doesn't seem like an optimization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes indeed, typeof window works in my experimental codes where window throws.

Thank you very much, I will update pull request according to your one liner above.

if (noWindow) {
return false;
}
if (theWindow === undefined) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

Try catch feels like overkill. Why not just typeof window != 'undefined'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It catches ReferenceError: window is not defined for --env=node in Jest or if pretty-format package is used for other purpose than testing.

Keep the questions coming. This one caused me to add comments in the code.

Copy link
Member

Choose a reason for hiding this comment

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

I added a new comment - it does not.

$ node -p 'typeof window'
undefined

Copy link
Member

@SimenB SimenB Oct 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Or well, the current test throws, but that's because the test itself is referring to window, not because it throws from inside pretty-format.

You can always typeof anything - even stuff that is not defined.

And this way you avoid the try-catch entirely

/* global window */
theWindow = window;
noWindow = false;
} catch (e) {
noWindow = true;
return false;
}
}
return val === theWindow;
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just:

const isGlobalWindow = val => {
  try {
    /* global window */
    return window === global;
  } catch (error) {
    return false;
  }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In answer to your question, must return result of comparison to val

The complexity is in case some overhead to enclose val === window in try-catch, lazy evaluation of window at first attempt to serialize an object.

The more I think about it though, considering global seems like a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should do a set of perf measurements compared to simplest possible code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just referring to make it a bit simpler (I now see I forgot to include val in the equation :D). If you could do some local perf testing, that's always nice 👍

const SYMBOL_REGEXP = /^Symbol\((.*)\)(.*)$/;
const NEWLINE_REGEXP = /\n/gi;

Expand Down Expand Up @@ -224,7 +244,9 @@ function printComplexValue(
'}';
}

return hitMaxDepth
// Avoid failure to serialize global window object in jsdom test environment.
// For example, not even relevant if window is prop of React element.
return hitMaxDepth || isWindow(val)
? '[' + (val.constructor ? val.constructor.name : 'Object') + ']'
: (min ? '' : (val.constructor ? val.constructor.name : 'Object') + ' ') +
'{' +
Expand Down