Modal relies on document as default renderNode, breaks SSR

[nolandg]

Line 117 of Modal.js uses document as the mount node if this.props.mountNode is absent. Thus if you don't specify mountNode, server-side rendering fails because there is no document.

Modal.js:117

The only workaround I can figure out is to track if the parent component has been mounted with a manually set this.state.hasMounted and then only render the Modal after componentDidMount has run. This makes the markup awkward and adds complexity.

Is there another option? Could Modal.js guard against trying to use document when it's not defined? Couldn't it just default to rendering in its parent like other components?

[levithomason]

The constraint here is that Semantic-UI's CSS requires an immediate child relationship from body > .ui.dimmer > .ui.modal.

In order to prevent a TypeError due to a missing a document I could see adding a safe guard. Though, I'd like to check on the best practices for dealing with this. I'm not sure if the responsibility lies on the component to check for a browser, or if it lies on the server to provide a virtual browser with something like jsdom.

[nolandg]

In terms of best practices: I'm using react-router and react-router-ssr which I believe is the major leading React SSR solution and the only one I hear anything about. They apparently do not provide jsdom and it's up to other code to render safely in a non-browser environment. SUIR is the first package to break my perfect SSR.

My only thoughts then are either patch the SUI CSS or make it Modal's responsibility to render after the parent has mounted. Or the way it is now and make every user guard Modal against rendering at the wrong moment. Am I missing any options?

[dcurletti]

Just wanted to point out that 9ae0b24 does not fix server side rendering meaning 3f4f7e5 is still broken.

ReferenceError: document is not defined
[1]     at Modal._this.getMountNode (.../node_modules/semantic-ui-react/dist/commonjs/modules/Modal/Modal.js:140:39)

[strues]

+1 to what @dcurletti said

MOUNT ERROR: ReferenceError: document is not defined
    at Modal._this.getMountNode (...node_modules/semantic-ui-react/dist/commonjs/modules/Modal/Modal.js:140:39)
    at Modal.render (.../node_modules/semantic-ui-react/dist/commonjs/modules/Modal/Modal.js:229:27)

I've yet to come across a work around outside of forgetting about using a modal.

[dcurletti]

@strues The easiest way to get around this is to render on the client.

Example:

import React from 'react';
import { Header, Icon, Modal } from 'semantic-ui-react';

class Test extends React.Component {
  state = {
    isMounted: false
  };

  componentDidMount() {
    this.setState({isMounted: true});
  };

  render() {
    return (
        <div>
          {
            this.state.isMounted &&
            <Modal trigger={<button className="ui primary button">Open Modal</button>}>
              This works
            </Modal>
          }
        </div>
    )
  }
}

[jeffcarbs]

@nolandg @dcurletti @strues - Given that this isn't fixed I'm going to reopen it. Would one of you be able to provide a minimal example that reproduces this issue? I'm not too familiar with server-side rendering so I don't even know how I'd go about trying to reproduce this.

A PR with a bug fix would also be welcome 😁 (cc @levithomason 😉)

[nolandg]

I'm using Meteor + react-router + react-router-ssr. Not exactly minimal for this issue. Is someone already using something simpler so I don't have to make something from scratch?

[levithomason]

@jcarbo it looks like the issue is that getMountNode returns document.body if this.props.mountNode was not passed in. This happens on initial render as it is called and passed to the Portal in the Modal render:

// abbreviated Modal render()
<Portal mountNode={this.getMountNode()} />

Based on the above feedback, the only issue we need to fix is that we should not try to access document when it doesn't exist. The actual render output of the Modal when there is no document appears to be irrelevant, so we need not worry about rendering at all.

Proposal

I think we should avoid setting state on mount as this is an antipattern. It also causes an unnecessary render and complicates testing. I propose instead we simply render null if there is no mount node, otherwise, render the Portal.

1. Default prop mountNode: typeof document === 'object' ? document.body : null

2. Short circuit Modal render() if there is no mountNode

   render() {
     const { mountNode } = this.props
     if (!mountNode) return null
   
     // return <Portal />
   }

3. Remove getMountNode and update all references to use the prop only

---

I'll ship this and see how it goes!

[levithomason]

@nolandg @dcurletti @strues I'm curious whether or not you're using other components that also access the document, such as the Dimmer, Dropdown, Search, Select, or Popup? If so, are you not seeing these issues with those components?

[levithomason]

I've added #806 to address this issue in all components that access document/window. Please take a look, if you can test that branch, it would be much appreciated as well.

[dcurletti]

@levithomason I just added Search (literally an hour ago) and I was not getting a document error.

I quickly glanced at #806 and I think it'll do the trick, but give me some time tonight, for sure by tomorrow, to test it.

Sorry I didn't add a test case, my current setup is a fork from here: https://github.com/erikras/react-redux-universal-hot-example and won't be that easy to create a test gist with.

[levithomason]

I'll hold off until you are able to test it, thanks much!

[levithomason]

I went ahead and released the potential fix in [email protected]. Let me know if there are further issues and we'll reinvestigate this.

[dcurletti]

@levithomason (sorry for the tardiness). Unfortunately this did not fix it. #806 crashes in a different place now and complains about the window.
Trace:

error given was: ReferenceError: window is not defined
/some/path/node_modules/semantic-ui-react/dist/commonjs/lib/isBrowser.js:10:97)

[dcurletti]

Shoot, I looked at this line the other day too and forgot to comment:
const hasWindow = typeof window === 'object' || window.self === window
On the server (in nodejs) typeof window === 'object' will evaluate to false and then try to evaluate window.self which then hard crashes.

[reergymerej]

This bypasses errors for server-side rendering by just not rendering the component when the window object is absent. That leads to

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server:

because the component is not rendered on the server, but it is rendered on the client. Is this expected?

[levithomason]

That is the current design, however, I'm not sure as to the best practice here. See:

https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/modules/Modal/Modal.js#L313

We cannot render the Modal as it requires a document.body node. There is no document in SSR so we therefore skip rendering entirely. I'm very much open to PRs implementing better ways of handling this.