Skip to content

Commit

Permalink
fix(Modal): fix errors with SSR (#2260)
Browse files Browse the repository at this point in the history
* fix: Modal SSR

Resolve Issue #2259

* Change isBrowser to function

Allow isBrowser to be overridden for SSR tests
  • Loading branch information
adam-26 authored and levithomason committed Oct 29, 2017
1 parent 9fd189c commit 454daaa
Show file tree
Hide file tree
Showing 14 changed files with 87 additions and 32 deletions.
8 changes: 4 additions & 4 deletions src/addons/Portal/Portal.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ class Portal extends Component {
this.mountPortal()

// Server side rendering
if (!isBrowser) return null
if (!isBrowser()) return null

this.rootNode.className = className || ''

Expand All @@ -372,13 +372,13 @@ class Portal extends Component {
}

mountPortal = () => {
if (!isBrowser || this.rootNode) return
if (!isBrowser() || this.rootNode) return

debug('mountPortal()')

const {
eventPool,
mountNode = isBrowser ? document.body : null,
mountNode = isBrowser() ? document.body : null,
prepend,
} = this.props

Expand All @@ -396,7 +396,7 @@ class Portal extends Component {
}

unmountPortal = () => {
if (!isBrowser || !this.rootNode) return
if (!isBrowser() || !this.rootNode) return

debug('unmountPortal()')
const { eventPool } = this.props
Expand Down
2 changes: 1 addition & 1 deletion src/addons/Responsive/Responsive.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export default class Responsive extends Component {
constructor(...args) {
super(...args)

this.state = { width: isBrowser ? window.innerWidth : 0 }
this.state = { width: isBrowser() ? window.innerWidth : 0 }
}

componentDidMount() {
Expand Down
4 changes: 2 additions & 2 deletions src/behaviors/Visibility/Visibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export default class Visibility extends Component {
}

static defaultProps = {
context: isBrowser ? window : null,
context: isBrowser() ? window : null,
continuous: false,
offset: [0, 0],
once: true,
Expand Down Expand Up @@ -196,7 +196,7 @@ export default class Visibility extends Component {
}

componentDidMount() {
if (!isBrowser) return
if (!isBrowser()) return
const { context, fireOnMount } = this.props

this.pageYOffset = window.pageYOffset
Expand Down
2 changes: 1 addition & 1 deletion src/lib/debug.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _debug from 'debug'
import isBrowser from './isBrowser'

if (isBrowser && process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
if (isBrowser() && process.env.NODE_ENV !== 'production' && process.env.NODE_ENV !== 'test') {
// Heads Up!
// https://github.com/visionmedia/debug/pull/331
//
Expand Down
4 changes: 2 additions & 2 deletions src/lib/eventStack/eventStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class EventStack {
// ------------------------------------

sub = (name, handlers, options = {}) => {
if (!isBrowser) return
if (!isBrowser()) return

const { target = document, pool = 'default' } = options
const eventTarget = this._find(target)
Expand All @@ -43,7 +43,7 @@ class EventStack {
}

unsub = (name, handlers, options = {}) => {
if (!isBrowser) return
if (!isBrowser()) return

const { target = document, pool = 'default' } = options
const eventTarget = this._find(target, false)
Expand Down
9 changes: 8 additions & 1 deletion src/lib/isBrowser.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import _ from 'lodash'

const hasDocument = typeof document === 'object' && document !== null
const hasWindow = typeof window === 'object' && window !== null && window.self === window

export default hasDocument && hasWindow
// eslint-disable-next-line no-confusing-arrow
const isBrowser = () => !_.isNil(isBrowser.override)
? isBrowser.override
: hasDocument && hasWindow

export default isBrowser
4 changes: 2 additions & 2 deletions src/modules/Dimmer/Dimmer.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,15 @@ export default class Dimmer extends Component {
static Dimmable = DimmerDimmable

handlePortalMount = () => {
if (!isBrowser) return
if (!isBrowser()) return

// Heads up, IE doesn't support second argument in add()
document.body.classList.add('dimmed')
document.body.classList.add('dimmable')
}

handlePortalUnmount = () => {
if (!isBrowser) return
if (!isBrowser()) return

// Heads up, IE doesn't support second argument in add()
document.body.classList.remove('dimmed')
Expand Down
3 changes: 3 additions & 0 deletions src/modules/Modal/Modal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ export interface ModalProps extends PortalProps {

/** Custom styles. */
style?: React.CSSProperties;

/** Element to be rendered in-place where the portal is defined. */
trigger?: React.ReactNode;
}

interface ModalComponent extends React.ComponentClass<ModalProps> {
Expand Down
14 changes: 10 additions & 4 deletions src/modules/Modal/Modal.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import cx from 'classnames'
import _ from 'lodash'
import PropTypes from 'prop-types'
import React from 'react'
import React, { isValidElement } from 'react'

import {
AutoControlledComponent as Component,
Expand Down Expand Up @@ -128,6 +128,9 @@ class Modal extends Component {
/** Custom styles. */
style: PropTypes.object,

/** Element to be rendered in-place where the portal is defined. */
trigger: PropTypes.node,

/**
* NOTE: Any unhandled props that are defined in Portal are passed-through
* to the wrapping Portal.
Expand Down Expand Up @@ -161,7 +164,7 @@ class Modal extends Component {
}

// Do not access document when server side rendering
getMountNode = () => (isBrowser ? this.props.mountNode || document.body : null)
getMountNode = () => (isBrowser() ? this.props.mountNode || document.body : null)

handleActionsOverrides = predefinedProps => ({
onActionClick: (e, actionProps) => {
Expand Down Expand Up @@ -311,11 +314,13 @@ class Modal extends Component {

render() {
const { open } = this.state
const { closeOnDimmerClick, closeOnDocumentClick, dimmer, eventPool } = this.props
const { closeOnDimmerClick, closeOnDocumentClick, dimmer, eventPool, trigger } = this.props
const mountNode = this.getMountNode()

// Short circuit when server side rendering
if (!isBrowser) return null
if (!isBrowser()) {
return isValidElement(trigger) ? trigger : null
}

const unhandled = getUnhandledProps(Modal, this.props)
const portalPropNames = Portal.handledProps
Expand Down Expand Up @@ -352,6 +357,7 @@ class Modal extends Component {
closeOnDocumentClick={closeOnDocumentClick}
closeOnRootNodeClick={closeOnDimmerClick}
{...portalProps}
trigger={trigger}
className={dimmerClasses}
eventPool={eventPool}
mountNode={mountNode}
Expand Down
2 changes: 1 addition & 1 deletion src/modules/Popup/Popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export default class Popup extends Component {
const style = { position: 'absolute' }

// Do not access window/document when server side rendering
if (!isBrowser) return style
if (!isBrowser()) return style

const { offset } = this.props
const { pageYOffset, pageXOffset } = window
Expand Down
2 changes: 1 addition & 1 deletion src/modules/Search/Search.js
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ export default class Search extends Component {
scrollSelectedItemIntoView = () => {
debug('scrollSelectedItemIntoView()')
// Do not access document when server side rendering
if (!isBrowser) return
if (!isBrowser()) return
const menu = document.querySelector('.ui.search.active.visible .results.visible')
const item = menu.querySelector('.result.active')
if (!item) return
Expand Down
6 changes: 3 additions & 3 deletions src/modules/Sticky/Sticky.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export default class Sticky extends Component {
active: true,
bottomOffset: 0,
offset: 0,
scrollContext: isBrowser ? window : null,
scrollContext: isBrowser() ? window : null,
}

static _meta = {
Expand All @@ -93,7 +93,7 @@ export default class Sticky extends Component {
}

componentDidMount() {
if (!isBrowser) return
if (!isBrowser()) return
const { active } = this.props

if (active) {
Expand All @@ -117,7 +117,7 @@ export default class Sticky extends Component {
}

componentWillUnmount() {
if (!isBrowser) return
if (!isBrowser()) return
const { active } = this.props

if (active) this.removeListeners()
Expand Down
37 changes: 27 additions & 10 deletions test/specs/lib/isBrowser-test.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,35 @@
import isBrowser from 'src/lib/isBrowser'

describe('isBrowser', () => {
it('should return true in a browser', () => {
// tests are run in a browser, this should be true
isBrowser.should.be.true()
})
describe('browser', () => {
it('should return true in a browser', () => {
// tests are run in a browser, this should be true
isBrowser().should.be.true()
})

it('should return false when there is no document', () => {
require('imports-loader?document=>undefined!src/lib/isBrowser').default().should.be.false()
require('imports-loader?document=>null!src/lib/isBrowser').default().should.be.false()
})

it('should return false when there is no document', () => {
require('imports-loader?document=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports-loader?document=>null!src/lib/isBrowser').default.should.be.false()
it('should return false when there is no window', () => {
require('imports-loader?window=>undefined!src/lib/isBrowser').default().should.be.false()
require('imports-loader?window=>null!src/lib/isBrowser').default().should.be.false()
})
})

it('should return false when there is no window', () => {
require('imports-loader?window=>undefined!src/lib/isBrowser').default.should.be.false()
require('imports-loader?window=>null!src/lib/isBrowser').default.should.be.false()
describe('server-side', () => {
before(() => {
isBrowser.override = false
})

after(() => {
isBrowser.override = null
})

it('should return override value', () => {
// tests are run in a browser, this should be true
isBrowser().should.be.false()
})
})
})
22 changes: 22 additions & 0 deletions test/specs/modules/Modal/Modal-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react'
import ReactDOMServer from 'react-dom/server'

import Modal from 'src/modules/Modal/Modal'
import ModalHeader from 'src/modules/Modal/ModalHeader'
Expand All @@ -9,6 +10,7 @@ import Portal from 'src/addons/Portal/Portal'

import { assertNodeContains, assertBodyClasses, assertBodyContains, domEvent, sandbox } from 'test/utils'
import * as common from 'test/specs/commonTests'
import isBrowser from 'src/lib/isBrowser'

// ----------------------------------------
// Wrapper
Expand Down Expand Up @@ -538,4 +540,24 @@ describe('Modal', () => {
})
})
})

describe('server-side', () => {
before(() => {
isBrowser.override = false
})

after(() => {
isBrowser.override = null
})

it('renders empty content when trigger is not a valid component', () => {
const markup = ReactDOMServer.renderToStaticMarkup(<Modal />)
markup.should.equal('')
})

it('renders a valid trigger component', () => {
const markup = ReactDOMServer.renderToStaticMarkup(<Modal trigger={<div id='trigger' />} />)
markup.should.equal('<div id="trigger"></div>')
})
})
})

0 comments on commit 454daaa

Please sign in to comment.