Skip to content

[WIP] manage location through MatchProvider #4067

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[WIP] manage location through MatchProvider #4067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7ea5600
manage location through MatchProvider
pshrmn Oct 19, 2016
ae92492
split first pass server tests
pshrmn Oct 21, 2016
1c7d1a2
use nextContext
pshrmn Oct 25, 2016
cd1c58d
remove LocationSubscriber from Match
pshrmn Oct 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 69 additions & 72 deletions modules/Match.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,103 +1,100 @@
import React, { PropTypes } from 'react'
import MatchProvider from './MatchProvider'
import matchPattern from './matchPattern'
import { LocationSubscriber } from './Broadcasts'

class RegisterMatch extends React.Component {
class Match extends React.Component {
static defaultProps = {
exactly: false
}

static contextTypes = {
match: PropTypes.object,
location: PropTypes.object,
provider: PropTypes.object,
serverRouter: PropTypes.object
}

registerMatch() {
const { match:matchContext } = this.context
const { match } = this.props
constructor(props) {
super(props)

if (match && matchContext) {
matchContext.addMatch(match)
this.state = {
match: null
}
}

componentWillMount() {
if (this.context.serverRouter) {
this.registerMatch()
this.update = (location) => {
const match = this.matchCurrent(location)
this.setState({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be optimized to check if the new match matches the current this.state.match and not calling this.setState if it does.

match
})
return match !== null
}
}

componentDidMount() {
if (!this.context.serverRouter) {
this.registerMatch()
}
matchCurrent(location) {
const {
pattern,
exactly
} = this.props
const { provider } = this.context
const parent = provider && provider.parent
return matchPattern(pattern, location, exactly, parent)
}

componentDidUpdate(prevProps) {
const { match } = this.context

if (match) {
if (prevProps.match && !this.props.match) {
match.removeMatch(prevProps.match)
} else if (!prevProps.match && this.props.match) {
match.addMatch(this.props.match)
}
}
componentWillMount() {
const loc = this.props.location || this.context.location
const match = this.matchCurrent(loc)
this.setState({
match
})

const { serverRouter, provider } = this.context
if (serverRouter && provider) {
serverRouter.updateMatchStatus(
provider.serverRouterIndex,
match !== null
)
this.unsubscribe = provider.addMatch(this.update)
}
}

componentWillUnmount() {
if (this.props.match) {
this.context.match.removeMatch(this.props.match)
componentDidMount() {
const { serverRouter, provider } = this.context
if (!serverRouter && provider) {
this.unsubscribe = provider.addMatch(this.update)
}
}

render() {
return React.Children.only(this.props.children)
}
}

if (__DEV__) {
RegisterMatch.propTypes = {
children: PropTypes.node.isRequired,
match: PropTypes.any
componentWillReceiveProps(nextProps, nextContext) {
const location = nextProps.location || nextContext.location
const match = this.matchCurrent(location)
this.setState({
match
})
}
}

class Match extends React.Component {
static defaultProps = {
exactly: false
}

static contextTypes = {
location: PropTypes.object,
match: PropTypes.object
componentWillUnmount() {
if (this.unsubscribe) {
this.unsubscribe()
}
}

render() {
const { children, render, component:Component,
pattern } = this.props
const { match } = this.state
const location = this.props.location || this.context.location
const props = { ...match, location, pattern }
return (
<LocationSubscriber>
{(locationContext) => {
const { children, render, component:Component,
pattern, location, exactly } = this.props
const { match:matchContext } = this.context
const loc = location || locationContext
const parent = matchContext && matchContext.parent
const match = matchPattern(pattern, loc, exactly, parent)
const props = { ...match, location: loc, pattern }
return (
<RegisterMatch match={match}>
<MatchProvider match={match}>
{children ? (
children({ matched: !!match, ...props })
) : match ? (
render ? (
render(props)
) : (
<Component {...props}/>
)
) : null}
</MatchProvider>
</RegisterMatch>
<MatchProvider location={location} match={match}>
{children ? (
children({ matched: !!match, ...props })
) : match ? (
render ? (
render(props)
) : (
<Component {...props}/>
)
}}
</LocationSubscriber>
) : null}
</MatchProvider>
)
}
}
Expand Down
54 changes: 28 additions & 26 deletions modules/MatchProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import React, { PropTypes } from 'react'
import {
matchContext as matchContextType
providerContext as providerContextType
} from './PropTypes'

class MatchProvider extends React.Component {
static childContextTypes = {
match: matchContextType.isRequired
provider: providerContextType.isRequired,
location: PropTypes.object.isRequired
}

static contextTypes = {
Expand All @@ -17,34 +18,35 @@ class MatchProvider extends React.Component {
// **IMPORTANT** we must mutate matches, never reassign, in order for
// server rendering to work w/ the two-pass render approach for Miss
this.matches = []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually breaks server rendering in its current form because that only looks at the number of elements in the matches array of a <MatchProvider>. The current test for that ('renders misses on second pass with server render context result') just doesn't fail because there are no <Match> components to be added to the matches array. I'll update that to be breaking so that that issue has to be addressed.

Copy link
Contributor

@alisd23 alisd23 Oct 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like server-rendering does work at the moment, that test is correct. The matches in the current version is an array of objects for matching <Match>'s, whereas you're making it an array of functions, 1 from <Match> component?

Did you mean this now breaks with the changes in this PR, or that it was broken originally?

Copy link
Contributor Author

@pshrmn pshrmn Oct 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add the 'doesn't render misses on first pass' test to the current v4 branch, it should fail.

If you look at the render function of <Miss>, it determines if it should render by calling the missedAtIndex function. The current missedAtIndex code just checks if the current item (determined by serverRouterIndex) has matches in the matchesByIdentity array and if there is a <Miss> component for it.

The second check will always be true because the <Miss> will have registered itself. The first check will be true (aka there are no matches) if no <Match> components match prior to the <Miss> component rendering. If there is <Match> that matches the current location, but it is rendered after a <Miss>, that <Miss> will be rendered on the first pass.

this.subscribers = []
this.hasMatches = null // use null for initial value
this.misses = []

this.serverRouterIndex = null
}

addMatch = match => {
this.matches.push(match)
addMatch = fn => {
this.matches.push(fn)
return () => {
this.matches.splice(this.matches.indexOf(fn), 1)
}
}

removeMatch = match => {
this.matches.splice(this.matches.indexOf(match), 1)
addMiss = fn => {
this.misses.push(fn)
return () => {
this.misses.splice(this.misses.indexOf(fn), 1)
}
}

getChildContext() {
return {
match: {
provider: {
addMatch: this.addMatch,
removeMatch: this.removeMatch,
addMiss: this.addMiss,
matches: this.matches,
parent: this.props.match,
serverRouterIndex: this.serverRouterIndex,
subscribe: (fn) => {
this.subscribers.push(fn)
return () => {
this.subscribers.splice(this.subscribers.indexOf(fn), 1)
}
}
}
serverRouterIndex: this.serverRouterIndex
},
location: this.props.location
}
}

Expand All @@ -56,7 +58,7 @@ class MatchProvider extends React.Component {
const { serverRouter } = this.context
if (serverRouter) {
this.serverRouterIndex =
serverRouter.registerMatchContext(this.matches)
serverRouter.registerMatchProvider(this.matches)
}
}

Expand All @@ -65,12 +67,11 @@ class MatchProvider extends React.Component {
}

notifySubscribers() {
// React's contract is that cDM of descendants is called before cDM of
// ancestors, so here we can safely check if we found a match
if (this.subscribers.length) {
this.hasMatches = this.matches.length !== 0
this.subscribers.forEach(fn => fn(this.hasMatches))
}
const { location } = this.props
const hasMatches = this.matches
.map(fn => fn(location))
.some(matches => matches)
this.misses.forEach(fn => fn(hasMatches))
}

render() {
Expand All @@ -81,7 +82,8 @@ class MatchProvider extends React.Component {
if (__DEV__) {
MatchProvider.propTypes = {
match: PropTypes.any,
children: PropTypes.node
children: PropTypes.node,
location: PropTypes.object
}
}

Expand Down
15 changes: 7 additions & 8 deletions modules/Miss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { location as locationType } from './PropTypes'

class Miss extends React.Component {
static contextTypes = {
match: PropTypes.object,
provider: PropTypes.object,
location: PropTypes.object,
serverRouter: PropTypes.object
}
Expand All @@ -12,8 +12,8 @@ class Miss extends React.Component {
super(props, context)

// ignore if rendered out of context (probably for unit tests)
if (context.match && !context.serverRouter) {
this.unsubscribe = this.context.match.subscribe((matchesFound) => {
if (context.provider && !context.serverRouter) {
this.unsubscribe = this.context.provider.addMiss((matchesFound) => {
this.setState({
noMatchesInContext: !matchesFound
})
Expand All @@ -22,7 +22,7 @@ class Miss extends React.Component {

if (context.serverRouter) {
context.serverRouter.registerMissPresence(
context.match.serverRouterIndex
context.provider.serverRouterIndex
)
}

Expand All @@ -40,11 +40,10 @@ class Miss extends React.Component {
render() {
const { render, component:Component } = this.props
const { noMatchesInContext } = this.state
const { location:locationProp } = this.props
const location = locationProp || this.context.location
const { serverRouter, match } = this.context
const location = this.props.location || this.context.location
const { serverRouter, provider } = this.context
const noMatchesOnServerContext = serverRouter &&
serverRouter.missedAtIndex(match.serverRouterIndex)
serverRouter.missedAtIndex(provider.serverRouterIndex)
if (noMatchesInContext || noMatchesOnServerContext) {
return (
render ? (
Expand Down
4 changes: 2 additions & 2 deletions modules/PropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ export const action = PropTypes.oneOf([
'POP'
])

export const matchContext = PropTypes.shape({
export const providerContext = PropTypes.shape({
addMatch: PropTypes.func.isRequired,
removeMatch: PropTypes.func.isRequired
addMiss: PropTypes.func.isRequired
})

export const history = PropTypes.shape({
Expand Down
3 changes: 1 addition & 2 deletions modules/StaticRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,9 @@ class StaticRouter extends React.Component {
render() {
const { location } = this.state
const { action, children } = this.props

return (
<LocationBroadcast value={location}>
<MatchProvider>
<MatchProvider location={location}>
{typeof children === 'function' ? (
children({ action, location, router: this.getRouterContext() })
) : (
Expand Down
9 changes: 6 additions & 3 deletions modules/__tests__/Match-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import expect from 'expect'
import React from 'react'
import Match from '../Match'
import MatchProvider from '../MatchProvider'
import { renderToString } from 'react-dom/server'
import { render } from 'react-dom'
import { LocationBroadcast } from '../Broadcasts'
Expand Down Expand Up @@ -368,9 +369,11 @@ describe('Match', () => {
const location = { pathname: '/', state: { test: TEXT } }
const html = renderToString(
<LocationBroadcast value={location}>
<Match pattern="/" render={({ location }) => (
<div>{location.state.test}</div>
)}/>
<MatchProvider location={location}>
<Match pattern="/" render={({ location }) => (
<div>{location.state.test}</div>
)}/>
</MatchProvider>
</LocationBroadcast>
)
expect(html).toContain(TEXT)
Expand Down
Loading