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

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

Open
ipeychev opened this issue Oct 1, 2018 · 2 comments
Open

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

ipeychev opened this issue Oct 1, 2018 · 2 comments
Labels

Comments

@ipeychev
Copy link
Contributor

ipeychev commented Oct 1, 2018

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
Copy link
Contributor

jbalsas commented Oct 1, 2018

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 jbalsas added the bug label Oct 1, 2018
@jbalsas
Copy link
Contributor

jbalsas commented Oct 1, 2018

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.

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

No branches or pull requests

2 participants