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

No longer possible to run tests for components which import plotly without a browser #1675

Closed
wayofthepie opened this issue May 11, 2017 · 3 comments · Fixed by #1683
Closed

Comments

@wayofthepie
Copy link

wayofthepie commented May 11, 2017

Recently a change was made to svg_text_utils.js which instantiates a DOMParser at the top level of that module:

var parser = new DOMParser();
.

This has broken a number of tests I have which do not use a browser (or jsdom etc..) because the DOMParser will be instantiated on import, giving the following:

var parser = new DOMParser();
                 ^

ReferenceError: DOMParser is not defined
    at Object.<anonymous> (/home/user/repo/node_modules/plotly.js/src/lib/svg_text_utils.js:22:18)
...

For now I've applied a very quick fix:

var parser;
function cachingParser() {
  parser = parser === undefined ? new DOMParser(): parser;
  return parser
};
...
var dom = cachingParser().parseFromString(skeleton, 'application/xml'),
        childNode = dom.documentElement.firstChild;
...

Can this be updated to something like the above? If not, then does the DOMParser really need to be at the top level?

@etpinard
Copy link
Contributor

Thanks for tracking down this bug.

I'm curious, what kind of tests are you running in node? I'm surprised that anything plotly.js works in node w/o at least using jsdom. That said, we should at the very least repalce that new DOMParser() statement with new window.DOMParser() so that it can be more easily mocked in non-browser runtimes.

If not, then does the DOMParser really need to be at the top level?

It doesn't need to. But, apparently instantiating DOMParser is a non-negligible operation so @hy9be moved it to the module scope in #1585 instead of instantiating it every time a text node is drawn.

@wayofthepie
Copy link
Author

I actually am using jsdom 😉 was half asleep yesterday when I came across this!

I can set up a DOMParser as is with jsDom, so all is good really for me, but ya maybe what you suggested would be better, to use new window.DOMParser() as window is generally simulated anyway.

@bnbarak
Copy link

bnbarak commented Jan 24, 2019

Temporary fix to run tests that import Plotly in a non-browser environment.

const importPlotly = () => {
  if(window.isTesting)
    return {Plot: null, Plotly: null} // or Plot: <div />

  const Plotly = require('plotly.js/dist/plotly.min.js');
  const Plot = require('react-plotly.js');
  return { Plot, Plotly };
};

const { Plot, Plotly } = importPlotly();

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

Successfully merging a pull request may close this issue.

3 participants