Don't assume HTMLElement is in the global scope #91

jetpacmonkey · 2017-03-22T20:05:43Z

Instead uses globalObject.HTMLElement, to avoid breaking in a node environment where HTMLElement is not defined.

Closes #98

Instead uses `globalObject.HTMLElement`, to avoid breaking in a node environment where `HTMLElement` is not defined.

keithamus · 2017-03-22T20:17:11Z

Thanks for the PR @jetpacmonkey. Could we also get some tests for this? Or some information on how you have come to produce the error?

meeber · 2017-03-22T20:19:45Z

@jetpacmonkey Could you provide a bit more detail about the problem you're running into, and the surrounding circumstances?

The lines you changed are currently only run when isDom is true. isDom is only true when globalObject contains a location and document property. So it sounds like you have an environment that is browser-like, but lacking HTMLElement. But if that's the case, then although changing it to globalObject.HTMLElement will stop an error from being thrown, since it'll now reference undefined, the resulting logic doesn't really make sense, as it translates to obj instanceof undefined.

jetpacmonkey · 2017-03-22T20:38:44Z

Hmm, you're right. I'm using jsdom in my test environment, and only put a few values from the window object (including location and document) into the global scope. I updated my dependency on Sinon, and suddenly all of my tests that use Sinon.match (which depends on this library) started breaking. I'd rather not copy the entirety of my jsdom window into global if I don't have to, since the writers of jsdom call it an antipattern.

keithamus · 2017-03-22T20:51:42Z

only put a few values from the window object (including location and document) into the global scope

Do you need these in the global scope, given as you've already said this is an antipattern?

jetpacmonkey · 2017-03-22T21:03:16Z

True, doing less of an antipattern is still an antipattern :)

I'll see if I can remember why I did that, and remove whatever's depending on it. Thanks for the quick responses!

keithamus · 2017-04-10T20:58:44Z

@jetpacmonkey did you get anywhere with that? I think unless there's a super solid usecase for this, I'd rather close the PR. Thoughts?

LeonardoGentile · 2017-04-12T09:13:12Z

I have the exact use case of @jetpacmonkey, I'm using jsdom, and whenever using sinon.match I get this error, see #98

LeonardoGentile · 2017-04-12T09:35:59Z

See also: enzymejs/enzyme#888

jetpacmonkey · 2017-04-14T19:40:39Z

I think enzyme might be the real problem here, since it expects to execute in a browser-like environment, and I haven't come across a great tool for sandboxing browser-like environments. Someone should write one of those...

Anyways, it appears that I don't have a great way to get around that antipattern. However, what's really confusing me right now is the fact that I'm not intentionally putting location onto the global scope in my test environment, as far as I can see. This is feeling more and more to me like a bad interaction between various testing libs, which would mean this PR probably isn't the best solution. But... I still don't know what the actual solution should be.

jetpacmonkey · 2017-04-14T20:56:25Z

This appears to be relevant: https://github.com/airbnb/enzyme/blob/master/docs/guides/jsdom.md#describewithdom-api-and-clearing-the-document-after-every-test

Antipattern or no, a shared global DOM appears to be required for testing React components.

LeonardoGentile · 2017-04-14T21:27:00Z

@jetpacmonkey I'm also not putting location into the global..

Resuming:
enzyme is suggesting us to do something that the jsdom docs tell us not to do saying that there is no better way at the moment to do this.

Now, apart from that, who is adding the location to the global? Apparently shouldn't be jsdom.
Mistery..

My configuration before loading the react tests:

const doc = jsdom.jsdom('<!doctype html><html><body></body></html>', {
  url: 'http://localhost'
});
const win = doc.defaultView;

// Setup for react test-utils 'renderIntoDocument':
// it requires window, window.document and window.document.createElement
// globally available before you can import React
global.document = doc;
global.window = win;
global.navigator = {
  userAgent: 'node.js'
};

EDIT:
Simply this 2 lines from type-detect explains the location thing:

var globalObject = typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : self;
var isDom = 'location' in globalObject && 'document' in globalObject;

Given I've added global.window = doc it means that for type-detect the globalObject = window so at this point it's normal that both location and document are properties of that globalObject (the window)

LeonardoGentile · 2017-04-14T23:15:32Z

@keithamus according to this comment from enzyme member he think this should be an issue of type-detect.

What do you think?

As temp fix until this will be solved I can only see an extra line in my config before running the tests:

// [...] continues from above comment
global.HTMLElement = win.HTMLElement;

jetpacmonkey · 2017-04-17T13:41:36Z

So, the initial reason for not merging this PR as it was presented was that obj instanceof undefined "doesn't make sense". However, it does avoid things unexpectedly breaking, and it doesn't cause any false positives. The only case where it has any effect at all is arguably an antipattern, but it's also a pattern that's explicitly recommended by at least one popular testing library.

Plus, I'd argue that relying on implicit globals any more than is absolutely necessary is an antipattern itself. @keithamus thoughts?

meeber · 2017-04-17T13:45:43Z

at work so cant get into detail, but essentially the pr needs to be updated so that the tests involving HTMLElement are skipped if its undefined. checking if it's defined should occur at the top with all the other checks.

…

On Apr 17, 2017 9:41 AM, "Jeremy Tice" ***@***.***> wrote: So, the initial reason for not merging this PR as it was presented was that obj instanceof undefined "doesn't make sense". However, it *does* avoid things unexpectedly breaking, and it doesn't cause any false positives. The only case where it has any effect at all is arguably an antipattern, but it's also a pattern that's explicitly recommended by at least one popular testing library. Plus, I'd argue that relying on implicit globals any more than is absolutely necessary is an antipattern itself. @keithamus <https://github.com/keithamus> thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#91 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQdhvd7NXKZ3PidrD78XpQBDPx5gcbKfks5rw2wQgaJpZM4MlwBg> .

jetpacmonkey · 2017-04-17T15:17:51Z

Alright, I can do that.

keithamus · 2017-04-18T05:32:40Z

I had two real concerns initially with the PR; one being I wasn't totally satisfied with why we should make the change, as it seems we have valid enough code. I'm satisfied now, however, that there is clear precedent for us to be more defensive.

My larger concern is that I'd like some tests to prevent regressions in future. However this is a simple enough change, which will be difficult to test, and has hung around for nearly a month so I'm willing to forgo the need for tests. I would however agree with @meeber - isDOM should probably also check for 'HTMLElement' in globalObject so skip those checks if we're in a faked out DOM like jsdom. Update this and I'm happy to merge.

jetpacmonkey · 2017-04-18T16:18:31Z

You know, I just re-read what @LeonardoGentile wrote. Checking if it's in the globalObject won't actually solve anything, because when a Node environment adds window to the global scope, globalObject isn't actually the global object, it's global.window. I really think relying on something to be in global scope instead of explicitly reading it from globalObject is a mistake.

jetpacmonkey · 2017-04-18T16:27:19Z

And I have no problem writing a couple of tests for this. I just wanted to make sure that this was a solution that you guys were ok with before I spent the time.

meeber · 2017-04-18T22:31:37Z

Upon further reflection, I think we should do two things here:

Improve type-detect's global object detection so that in Node it always uses global as the global object, even if global.window is defined. According to this post, a reliable option is: var globalObject = Function('return this')();
Make it so that browser-related type checks are only performed if ALL browser-related globals being used by type-detect are defined. If this is done, then it doesn't matter if HTMLElement or globalObject.HTMLElement is used for the actual equality comparison, since the equality comparison will only occur in the first place if HTMLElement has already been verified to exist.

shvaikalesh · 2017-04-18T22:45:11Z

Function('return this')();

has CSP issues, otherwise 👍

jetpacmonkey · 2017-04-19T19:34:14Z

@meeber I pushed up a check that should do what you described. I suppose for the new tests, I'll just add a jsdom devDependency and re-run the node tests using that?

jetpacmonkey · 2017-04-19T19:36:20Z

The funny thing is, with my original change, it actually wouldn't have been obj instanceof undefined, since globalObject was the jsdom window, so globalObject.HTMLElement probably would have been defined.

jetpacmonkey · 2017-04-19T19:37:53Z

Oh, and apparently the linter hated that commit. Looks like it was disabled for that line anyways, before, so I guess I'll do that again.

meeber · 2017-04-19T21:10:50Z

index.js

+// See http://stackoverflow.com/a/6930376
+var globalObject;
+try {
+  globalObject = Function('return this')() || (42, eval)('this');


It's my understanding that all that's needed here is var globalObject = Function('return this')();. Not the || nor the try/catch.

On top of what @meeber has mentioned, I would prefer new Function. new with constructors is the new black in ES6. Also, this won't work in Chrome OS extensions for example. More on CSP: jashkenas/underscore#906.

This was, for the most part, copied from that StackOverflow post. The try/catch is needed because of CSP. window is a safe fallback because CSP doesn't exist in non-browser environments, afaik. The || I wasn't quite as clear on. Something to do with use strict if I was reading that SO post correctly?

@shvaikalesh will it "not work" even with the try/catch? I thought I was already accounting for CSP... Also, I agree about using new.

meeber · 2017-04-19T21:13:10Z

index.js

+  'document',
+  'navigator',
+  'HTMLElement',
+];


One thing I'm concerned about here is that lines like this if (obj === (globalObject.navigator || {}).mimeTypes) { indicate that sometimes globalObject.navigator isn't expected to be defined, even in a valid browser environment. However, this isn't my area of expertise. @keithamus @shvaikalesh thoughts?

I've never encountered a browser (not SSR nor React Native) environment without navigator defined.

meeber · 2017-04-19T21:19:48Z

@jetpacmonkey Thanks for working on this! Regarding tests: not sure how I feel about pulling in jsdom as a dependency. I think it's reasonable to fix this specific problem, but making "all tests must pass in jsdom" an absolute requirement for all future builds is a bit of a leap at this point. I'm not really sure at the moment what I think the best strategy is here. I'm not opposed to moving forward without additional automated tests for this. Thoughts @keithamus @shvaikalesh?

shvaikalesh · 2017-04-19T21:52:57Z

index.js

-var globalObject = typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : self; // eslint-disable-line
-var isDom = 'location' in globalObject && 'document' in globalObject;
+
+/* eslint-disable */


I would prefer to disable specific rule(s), not the whole eslint.

Sure. I just did it this way because it was originally // eslint-disable-line

Modified PR

keithamus · 2017-09-06T20:49:32Z

Okay I've modified the PR to fix up all the comments. I'm happy with this but I'll dismiss my PR as I've contributed. @shvaikalesh @meeber could you please check through this and if we're all happy with this we can squash and merge as a fix: commit to get it released.

shvaikalesh · 2017-09-06T20:53:41Z

I think we should avoid new Function (because of CSP) and do something like var globalObject = typeof self === 'object' ? self : global.

keithamus · 2017-09-06T21:03:42Z

Okay @shvaikalesh how about this? Best of both worlds?

meeber · 2017-09-06T21:30:37Z

@shvaikalesh @keithamus Can you talk me through how the global object detection now works?

shvaikalesh · 2017-09-06T21:34:36Z

index.js

+  'HTMLElement' in window
+) {
+  isDom = true;
+  globalObject = window; // eslint-disable-line no-undef


I think we can remove this line: if we have window object, it is equal to self

shvaikalesh · 2017-09-06T21:37:40Z

@meeber

current code:

var globalObject = typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : self;

PR:

var globalObject = typeof self === 'object' ? self : global;

Kinda the same thing, except that the latter option relies on self == window.

keithamus · 2017-09-07T08:56:20Z

Okay, updated. We should be good now right? Whoever merges don't forget to squash+merge as fix:

keithamus · 2017-09-07T09:28:59Z

index.js

-var globalObject = typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : self; // eslint-disable-line
-var isDom = 'location' in globalObject && 'document' in globalObject;
+
+/* eslint-disable no-undef */


Oops, forgot to re-enable this one. Can't right now - but will get to it later.

mroderick · 2017-10-05T06:51:41Z

The build failed because of ESLint errors

/home/travis/build/chaijs/type-detect/index.js
   9:47  error  'self' is not defined    no-undef
  16:17  error  'window' is not defined  no-undef
  17:18  error  'window' is not defined  no-undef
  18:20  error  'window' is not defined  no-undef

It would be easy to fix this with a few well placed comments:

// eslint-disable-next-line no-undef id-blacklist
var globalObject = typeof self === 'object' ? self : global;
/*
 * All of these attributes must be available on the global object for the current environment
 * to be considered a DOM environment (browser)
 */
/* eslint-disable no-undef */
var isDom = typeof window === 'object' &&
  'document' in window &&
  'navigator' in window &&
  'HTMLElement' in window;
/* eslint-enable no-undef */

Who can fix the branch, so we can get this merged?

keithamus · 2017-10-18T16:43:15Z

@meeber @shvaikalesh shall we get this reviewed and (squash) merged?

meeber

Selecting "Approve" so if my question is off-base, we can get this merged without any further delay:

Is there any plausible scenario in which isDom gets set to true (because window is an object and has the document, navigator, and HTMLElement properties), but globalObject gets set to something other than window on line 11? If so, then the code won't behave as expected.

I guess I'm mostly worried about these "browser-like" Node environments thwarting the detection logic again, which is what led to this issue originally

keithamus · 2017-10-19T09:54:54Z

Is there any plausible scenario in which isDom gets set to true (because window is an object and has the document, navigator, and HTMLElement properties), but globalObject gets set to something other than window on line 11? If so, then the code won't behave as expected.

Perhaps this is a bug, like the one is this PR, which we'll cross when someone experiences it. While technically possible, I doubt we'll ever see it - if someone is trying to emulate the DOM it's probably a mistake to be missing self.

mroderick · 2017-10-31T12:41:22Z

Thank you 💯

It would be great if someone could publish a new release to npm

gpoole · 2017-11-08T01:24:36Z

It looks like sinonjs/sinon#1377 is waiting on this being released, is it possible to get it published please? 🙏

vieiralucas · 2017-11-08T02:14:55Z

@gpoole I think we can make a release as soon as #112 gets merged, which should be soon.

gpoole · 2017-11-08T02:49:48Z

Ah perfect, thanks @vieiralucas!

vieiralucas · 2017-11-09T13:17:02Z

@gpoole @chaijs/type-detect v4.0.4 just got released

Don't assume HTMLElement is in the global scope

d124dbd

Instead uses `globalObject.HTMLElement`, to avoid breaking in a node environment where `HTMLElement` is not defined.

LeonardoGentile mentioned this pull request Apr 14, 2017

ReferenceError: HTMLElement is not defined #98

Closed

More strenuous checks for if we're in a browser env

a0e89b7

style: Make the linter happy

b65aa15

meeber reviewed Apr 19, 2017

View reviewed changes

shvaikalesh reviewed Apr 19, 2017

View reviewed changes

keithamus mentioned this pull request Aug 21, 2017

expect(object).to.be.an('object') failing with 'ReferenceError: HTMLElement is not defined' chaijs/chai#1029

Closed

clean up code

9b2d462

jetpacmonkey requested a review from a team as a code owner September 6, 2017 20:30

keithamus added 2 commits September 6, 2017 21:35

fix up eslint workarounds

5336711

fix up eval hacks to get "this"

5eb1885

safely get window object without eval

21ce92b

shvaikalesh reviewed Sep 6, 2017

View reviewed changes

remove globalObject = window

b610c22

simplify isDom definition

c426e32

keithamus reviewed Sep 7, 2017

View reviewed changes

keithamus force-pushed the patch-1 branch from 35d42a2 to c426e32 Compare October 18, 2017 16:29

style(lint): re-enable no-undef rule inside index.js

971ce2c

meeber approved these changes Oct 19, 2017

View reviewed changes

meeber merged commit e7aa747 into chaijs:master Oct 31, 2017

meeber mentioned this pull request Nov 1, 2017

Problem releasing new version #109

Closed

Don't assume HTMLElement is in the global scope #91

Don't assume HTMLElement is in the global scope #91

Conversation

jetpacmonkey commented Mar 22, 2017 • edited by keithamus Loading

keithamus commented Mar 22, 2017 • edited Loading

meeber commented Mar 22, 2017

jetpacmonkey commented Mar 22, 2017

keithamus commented Mar 22, 2017 • edited Loading

jetpacmonkey commented Mar 22, 2017

keithamus commented Apr 10, 2017

LeonardoGentile commented Apr 12, 2017

LeonardoGentile commented Apr 12, 2017

jetpacmonkey commented Apr 14, 2017

jetpacmonkey commented Apr 14, 2017

LeonardoGentile commented Apr 14, 2017 • edited Loading

LeonardoGentile commented Apr 14, 2017

jetpacmonkey commented Apr 17, 2017

meeber commented Apr 17, 2017 via email

jetpacmonkey commented Apr 17, 2017

keithamus commented Apr 18, 2017 • edited Loading

jetpacmonkey commented Apr 18, 2017

jetpacmonkey commented Apr 18, 2017

meeber commented Apr 18, 2017

shvaikalesh commented Apr 18, 2017

jetpacmonkey commented Apr 19, 2017

jetpacmonkey commented Apr 19, 2017

jetpacmonkey commented Apr 19, 2017

meeber Apr 19, 2017

Choose a reason for hiding this comment

shvaikalesh Apr 19, 2017

Choose a reason for hiding this comment

jetpacmonkey Apr 20, 2017

Choose a reason for hiding this comment

meeber Apr 19, 2017

Choose a reason for hiding this comment

shvaikalesh Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

meeber commented Apr 19, 2017

shvaikalesh Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

jetpacmonkey Apr 20, 2017

Choose a reason for hiding this comment

keithamus commented Sep 6, 2017

shvaikalesh commented Sep 6, 2017

keithamus commented Sep 6, 2017

meeber commented Sep 6, 2017

shvaikalesh Sep 6, 2017

Choose a reason for hiding this comment

shvaikalesh commented Sep 6, 2017

keithamus commented Sep 7, 2017 • edited Loading

keithamus Sep 7, 2017

Choose a reason for hiding this comment

mroderick commented Oct 5, 2017

keithamus commented Oct 18, 2017 • edited Loading

meeber left a comment

Choose a reason for hiding this comment

keithamus commented Oct 19, 2017

mroderick commented Oct 31, 2017

gpoole commented Nov 8, 2017

vieiralucas commented Nov 8, 2017 • edited Loading

gpoole commented Nov 8, 2017

vieiralucas commented Nov 9, 2017

jetpacmonkey commented Mar 22, 2017 •

edited by keithamus

Loading

keithamus commented Mar 22, 2017 •

edited

Loading

keithamus commented Mar 22, 2017 •

edited

Loading

LeonardoGentile commented Apr 14, 2017 •

edited

Loading

keithamus commented Apr 18, 2017 •

edited

Loading

shvaikalesh Apr 20, 2017 •

edited

Loading

shvaikalesh Apr 19, 2017 •

edited

Loading

keithamus commented Sep 7, 2017 •

edited

Loading

keithamus commented Oct 18, 2017 •

edited

Loading

vieiralucas commented Nov 8, 2017 •

edited

Loading