Skip to content

Failing test for <Miss /> with 'Blocker' component #4047

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

Closed
wants to merge 1 commit into from

Conversation

alisd23
Copy link
Contributor

@alisd23 alisd23 commented Oct 14, 2016

Refers to issue #4035

@alisd23 alisd23 added the bug label Oct 14, 2016
@alisd23 alisd23 changed the base branch from master to v4 October 14, 2016 12:01
@alisd23 alisd23 force-pushed the miss-location-subscriber branch 2 times, most recently from 426be31 to 6a380c8 Compare October 14, 2016 12:05
done()
})
})
})

describe('FAILING MISS TESTS', (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

done should be an argument to the it callback, not describe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good catch, thanks

@ryanflorence
Copy link
Member

alright, I'm digging in on this today

@ryanflorence
Copy link
Member

afaict Miss works fine on master since we added the <LocationSubscriber> to it, I think the test is trolling you, if you click around the examples it works fine. Feel free to reopen if you find something, I'm just cleaning house here.

@alisd23 alisd23 reopened this Oct 25, 2016
@alisd23 alisd23 force-pushed the miss-location-subscriber branch from c095332 to 84f5597 Compare October 25, 2016 13:31
@alisd23 alisd23 added this to the v4.0.0 milestone Oct 25, 2016
@alisd23
Copy link
Contributor Author

alisd23 commented Oct 25, 2016

Updated to current v4 master, and test still fails. Same issue with <Blocker> causing removeMatch to be called too late, which means notifySubscribers has already fired with the incorrect match information and <Miss> has already re-rendered.

Mentioned in more detail in #4035.
This PR fixes the issue I believe - #4067

@ryanflorence
Copy link
Member

I know the test fails, but if you click around the examples w/ a <Blocker/> it still works. Can you confirm?

@pshrmn
Copy link
Contributor

pshrmn commented Oct 26, 2016

Try this app adapted from the test. When you navigate directly to / or /other, they render as expected, but when clicking the links they don't.

import React from 'react'
import { Match, Miss, Link, HashRouter } from 'react-router'

class Blocker extends React.Component {
  shouldComponentUpdate() {
    return false
  }

  render() {
    return <App />
  }
}

const Home = () => (
  <Link to='/other'>Other</Link>
)

const NotFound = () => (
  <div>
    <Link to='/'>Home</Link>
    <p>Not Found</p>
  </div>
)

const App = () => (
  <div>
    <Match pattern='/' exactly component={Home} />
    <Miss component={NotFound} />
  </div>
)

export default () => (
  <HashRouter>
    <Blocker />
  </HashRouter>
);

@alisd23 alisd23 removed this from the v4.0.0 milestone Nov 1, 2016
@alisd23 alisd23 added the v4 label Nov 1, 2016
@timdorr timdorr removed the v4 label Nov 1, 2016
@timdorr timdorr added this to the v4.0.0 milestone Nov 1, 2016
@alisd23 alisd23 force-pushed the miss-location-subscriber branch from 84f5597 to d455ca2 Compare November 7, 2016 09:39
@alisd23
Copy link
Contributor Author

alisd23 commented Nov 7, 2016

Test still failing when up to date in v4 branch (alpha-5)

There was a potential fix in #4067, but it was closed. Also maybe #4136 is related?

@pshrmn
Copy link
Contributor

pshrmn commented Nov 8, 2016

FWIW this isn't an issue with <MatchGroup> 😉

@mjackson
Copy link
Member

I'm afraid this PR is a bit outdated. <Miss> is gone in current v4, huzzah!

@mjackson mjackson closed this Jan 11, 2017
@alisd23
Copy link
Contributor Author

alisd23 commented Jan 11, 2017

👍

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants