edge case with jsdom and platform detection

[GabiGrin]

Hi I'm testing a component that uses react-popover inside it. For testing, we're using mocha and jsdom, but initializing jsdom only in the scope of the test, something along these lines like this:

it('should work', () => {
   const cleanup = require('jsdom-global')(); // or any other way of setting it up
   const elem = renderComponentIntoBody(<MyComponentWithPopover/>);
   // check some conditions
   cleanup();
});

The test runs using mocha, along with other server only tests, thus jsdom is not always loaded, but loaded on demand inside the UI specific ones.

The problem is that if the popover module is required before jsdom is set up, I get this error

 Invariant Violation: _registerComponent(...): Target container is not a DOM element.
      at invariant (node_modules/fbjs/lib/invariant.js:44:15)
      at Object.ReactMount._renderNewRootComponent (node_modules/react/lib/ReactMount.js:274:69)
      at Object.ReactMount._renderSubtreeIntoContainer (node_modules/react/lib/ReactMount.js:368:32)
      at Object.ReactMount.render (node_modules/react/lib/ReactMount.js:390:23)

After a long session of debugging, it was caused because react-popover mistakenly thought it was running on a server environment - https://github.com/littlebits/react-popover/blob/master/lib/react-layer-mixin.js#L9 While it was running in a "client" environment, or at least had a global document to work it. In other words - while everything is running in "client" mode, react-popover thinks it's in a server mode.

The root of the problem seems to be that https://github.com/littlebits/react-popover/blob/master/lib/platform.js will "decide" if it's a server or client env. when required, and not on demand, and you can not rely on require order nowdays with everyone mixing backend and frontend code :)

A possible fix is changing this file - https://github.com/littlebits/react-popover/blob/master/lib/platform.js to return functions and not a static data.

I'll test in a fork and open a PR and hopefully this will help others encountering this issue

[GabiGrin]

Tested in a fork and npm installed from it and it seems to work https://github.com/GabiGrin/react-popover I can clean it a bit and open a PR if this sounds reasonable

[GabiGrin]

can this be added? are you ok with the suggested solution? if so I'll open a PR

[jasonkuhrt]

@GabiGrin Hey, thanks for taking the time to look into this. Interesting! I wonder if the dynamism you are after is the right solution. Is there a way you could simplify your tests so that the static environment assessment of react-popover needn't become dynamic? Maybe split client and server tests into separate suites? I'm not 100% clear on your test setup so maybe you can clarify it a bit more. Cheers!

[jasonkuhrt]

Since this edge case only hits simulated test environments I'm no longer marking this as a bug.