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

Fix: server rendering by checking document and window object are defined #117

Merged
merged 1 commit into from
Oct 13, 2015
Merged

Conversation

sujaykathrotia
Copy link
Contributor

Server side rendering was not working as the JavaScript threw the Reference Error.
Check this issue: Hacker0x01/react-datepicker#179

@bruno12mota
Copy link

+1 for this

@ivarconr
Copy link

+1

@geekjuice
Copy link
Contributor

Should Tether even execute for server-side rendering? If the assumption is that Tether shouldn't even execute on the server, would it make sense to just wrap the entire file in a function that checks that document and window are defined? For example:

let Tether;
if (typeof window !== 'undefined' && typeof document !== 'undefined') {

    if (typeof TetherBase === 'undefined') {
      throw new Error('You must include the utils.js file before tether.js');
    }

    const {
      getScrollParent,
      getBounds,
      getOffsetParent,
      extend,
      addClass,
      removeClass,
      updateClasses,
      defer,
      flush,
      getScrollBarSize
    } = TetherBase.Utils;

    ...
}

@sujaykathrotia
Copy link
Contributor Author

I agree. Tether should not execute on the SSR. But then if someone has used object methods like setOptions, then it would throw the error. So I think it is better to check for windows and document only when we use them.

I am fine with either option though.

@sujaykathrotia
Copy link
Contributor Author

An alternative of wrapping the entire file in an if condition, would be to check if window is defined at the top of utils.js:

if (typeof window === 'undefined') {
  return;
}

Since the output file is wrapped in a factory function, just the return statement would do the trick.

@geekjuice
Copy link
Contributor

So after dwelling on this for a bit, I think the current implementation in this PR is fine for now since those are the only places where document and window gets called immediately due to them being inside IIFE.

Personally, I'm not completely satisfied with the solution, but it is a solution nonetheless and one that does seems to solve the issue well. Ideally, I would like to short-circuit the entire library when window and document are undefined, but that may require some restructuring of the library.

Thanks again for your patience 👍

geekjuice added a commit that referenced this pull request Oct 13, 2015
Fix: server rendering by checking document and window object are defined
@geekjuice geekjuice merged commit d480376 into shipshapecode:master Oct 13, 2015
@sujaykathrotia
Copy link
Contributor Author

@geekjuice The last solution I suggested would short-circuit the entire library without restructuring of the library.

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 this pull request may close these issues.

4 participants