metal / metal.js

Build UI components in a solid, flexible way
http://metaljs.com
Other
229 stars 59 forks source link

Metal Dom uses `document` but in Node there is no such thing #398

Open ipeychev opened 6 years ago

ipeychev commented 6 years ago

This commit uses document, but in Node there is no such thing. This breaks most of the tests of Marble components.

FAIL  packages/marble-tooltip/test/Tooltip.node.js
  ● Test suite failed to run

    ReferenceError: document is not defined

      at Function.checkAnimationEventName_ (node_modules/metal-dom/lib/features.js:55:34)
      at Function.checkAnimationEventName (node_modules/metal-dom/lib/features.js:35:26)
      at node_modules/metal-dom/lib/events.js:52:38
          at Array.forEach (<anonymous>)
      at registerEvents (node_modules/metal-dom/lib/events.js:43:33)
      at Object.<anonymous> (node_modules/metal-dom/lib/events.js:58:2)
      at Object.<anonymous> (node_modules/metal-dom/lib/all/dom.js:46:1)
jbalsas commented 6 years ago

Hi @ipeychev!

If I remember correctly, in the commit you linked, we removed the usage of document globally, so it wouldn’t fail when simply imported.

We did this under the assumption that if you used this API checkAnimatinoEventName you were making it safely from a browser environment, and the calling code would be wrapping it in an isServerSide if clause.

Based on the stacktrace, though, it looks like this is being called from events inside metal-dom, so we will take a look.

jbalsas commented 6 years ago

As I mentioned, the usage of this can be found in events.js and is:

if (!isServerSide()) {
    registerEvents();
}

Seeing this error, this might be related to Consider removing NODE_ENV check from isServerSide mismatching the environment.