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

ReferenceError: document is not defined #12

Closed
joshhornby opened this Issue Jul 13, 2015 · 22 comments

Comments

Projects
None yet
7 participants
@joshhornby
Copy link
Contributor

joshhornby commented Jul 13, 2015

doc = document,
                  ^
ReferenceError: document is not defined

Seems to be an issue when trying to run this using an isomorphic app

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 13, 2015

Seems this is related to #4. Pulling in the official repo seems to have the correct shims

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 13, 2015

Can you give me more feedback about this issue? When/where/way to reproduce?

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 14, 2015

@Kureev Yes sure, I will try and get an example repo showing the issue later today but the core issue is that Highcharts wants the document, window and navigator and because of the server side rendering accept of a isomorphic application these aren't available and thus It needs to be shimmed.

A shimmed version of highstocks is available here

https://github.com/jessegavin/highstock-module

And I propose we do something similar for this library.

I will try and get an example repo online later today giving an example of this issue.

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 14, 2015

Please see an example of the error here

https://github.com/joshhornby/highcharts-error-example

In order to run the example

npm install
npm run build
npm start

You will then see the document is not defined error.

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 14, 2015

I have also tried to do the following my editing the package, using webpack to rebuild and then npm link

global.document = {createElement:function(name){ return {getContext:function(){}} },documentElement:{}};
global.HighchartsAdapter = require('exports?HighchartsAdapter!Highcharts/js/adapters/standalone-framework.src');
global.navigator = {userAgent:'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/33.0.1750.3 Safari/537.36'};
global.window = {navigator:navigator,Date:Date};
var Highcharts = require('exports?Highcharts!Highcharts');
var React = require('react');
var update = require('react/addons').addons.update;

In order to shim the correct areas as a temp hack, this returns the error

        module.exports = Highcharts
                         ^
ReferenceError: Highcharts is not defined

sorry for the multiple emails but I am very very keen to get this issue resolved asap. :)

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 15, 2015

So, first of all - no worries, we'll solve this issue :)
I'm sure, that correct include of jsdom gonna solve this problem. Unfortunately, my webpack skills are not so good to make you a fast solution in code, but I can lead you a way:

  • Include jsdom for backend
  • Exclude jsdom bundling for the frontend
  • Incapsulate jsdom's mock to Highcharts (link)

Also, it does make sense include "index.jsx" from react-highcharts instead of using default entry point. In this case you can try to fork (or wait me to fix) react-highcharts and include jsdom by default + mock it for the browser. As I would have a time, I'll try to solve this. cc @kirjs

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 15, 2015

Thanks @Kureev Do you have an ETA on this or is this something you'd like me to look at? Thanks very much for your help

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 15, 2015

I'll try to figure out today, but can't promise. In theory we should supply a version for backend rendering with jsdom under the hood. But the problem is how you gonna bundle it. I need to have a deep look into webpack.

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 15, 2015

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 15, 2015

That's extremely useful, hope it'll work. That's what exactly I was looking for

@0xCMP

This comment has been minimized.

Copy link

0xCMP commented Jul 15, 2015

HI guys, I'm also having this issue. We rolled our own Highcharts component to use for isomorphic, but I believe it's causing some re-render issues. Did this turn out to be a successful approach?

I can't tell from the chat where some of the things to determine node vs browser are going to be (or already are) defined. Wondering if it's worth the time to figure out.

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 16, 2015

Hey @0xCMP I know @Kureev is actively trying to solve this issue.

Do you think you could share your own approach as I would love to see what you've got. Maybe I could help with the re-render issues.

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 16, 2015

So, I have some feedback. Yesterday I tried to solve this issue, but it seems it's easier to rewrite highcharts, than make everything we have now work fine on the server. Let me explain why:

  • jsdom related to contextify, which makes it impossible to bundle normally
  • It's not possible to "just make a stupid mock" for document, window and navigator, it really relays on their functionality. Simply put, it must work if we want normal highcharts rendering.

I also tried to use check for window in the env and based on that detect if I need to include lib or not and then exclude jsdom from bundle, but no luck so far: if I exclude it, I can't render it outside of the browser, so it'll lead us to different packages for browser and client solution, which doesn't suit the idea of isomorphic apps.

So, here I'll write some pitfalls I met when was investigating it. If somebody want try again, you'll be noticed what you should be aware for:

  • jsdom will never be bundled because of contextify. You can try jsdom-no-contextify, but it doesn't support native events which are needed for highcharts
  • browserify/webpack - doesn't really matter. With browserify you goes a bit deeper, but still no effect.
  • I've tried cheerio, jsdom, jsdom-no-contextify - nothing works correct

I'd say you should prevent rendering some code on backend, it doesn't worth it.

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 16, 2015

@Kureev Sounds like in its current state the library wont work on server rendered apps. If there was to be a rewrite what would change? As you mentioned above Highchart needs the document functionality to work so guessing not much could be done here?

@0xCMP Have you seen this fork? eleung@67c40dc#diff-43a37b970bfcb5981a4cbad9e7cef4b7R14 Might help with some of your re-rendering issues? Like I mentioned above if you could share your component that would be great.

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Jul 16, 2015

Yep, exactly. Actually, Highcharts doesn't even have npm package and that's make sense if they're not support backend rendering. It's just a trick to make a shim and use with react-highcharts, but it's still doesn't allow you to render it on the backend, unfortunately :(

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 16, 2015

There is https://github.com/jessegavin/highstock-module, Yes this is a highstock module but updating the gulp file would allow for the correct shims to be in place for Highcharts. Would it not be a case of slotting this in instead of https://github.com/kirjs/react-highcharts/blob/master/src/Highcharts.jsx#L1

@joshhornby

This comment has been minimized.

Copy link
Contributor Author

joshhornby commented Jul 16, 2015

With the help of @Kureev I seem to hack a (hacky) fix

import React from 'react';

export default class HighchartGraph extends React.Component {

    render() {
      let graph;

      if(window === undefined) {
        graph = (<div></div>);
      } else {
        var Highcharts = require('react-highcharts/more');
        graph = (<Highcharts config={this.props.chartData}></Highcharts>)
      }

      return (
          <div>
            {graph}
        </div>
      );
    };
}

This seems to have fixed the issue. I would still love to see @0xCMP custom component though :)

@Lngramos

This comment has been minimized.

Copy link

Lngramos commented Aug 17, 2015

I'm also having encountering the same issues here when rendering this on the backend – have there been any further findings since? ^^

@Kureev

This comment has been minimized.

Copy link
Collaborator

Kureev commented Aug 17, 2015

Nope, unfortunately nothing has been change in Highcharts since my last comment in this thread. AFAIK they are not going to provide npm package, because it leads to the whole library refactoring. @joshhornby proposed very viable solution for this moment.

@asyncb

This comment has been minimized.

Copy link

asyncb commented Jul 9, 2016

libraries like this should definitely accommodate server-rendering if they want to stay relevant

@kirjs

This comment has been minimized.

Copy link
Owner

kirjs commented Jul 11, 2016

@mrwillis

This comment has been minimized.

Copy link

mrwillis commented Sep 20, 2017

@kirjs Could you possibly provide a more complete example? For instance, where would you put that line? In index.js? Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment