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

Support history v5 #42

Merged
merged 2 commits into from
Jul 15, 2020
Merged

Support history v5 #42

merged 2 commits into from
Jul 15, 2020

Conversation

Hypnosphi
Copy link
Contributor

@Hypnosphi Hypnosphi commented Jul 14, 2020

Listener signature has changed from (location, action) => {...} to ({location, action}) => {...}

@Treora
Copy link
Owner

Treora commented Jul 15, 2020

Thanks for this PR; it’s good to keep up with the history module indeed.

Listener signature has changed from (listener, action) => {...} to ({listener, action}) => {...}

I wonder, how does that relate to the single change you committed?

    // Support history v5
    if (location.location != null) {
        location = location.location;
    }

Also I don’t see significant breaking changes on the module’s release page. Could you explain the need?

@Hypnosphi
Copy link
Contributor Author

Hypnosphi commented Jul 15, 2020

Unfortunately, there's a lot of unlisted breaking changes: remix-run/history#811

history.listen() API changed. The callbacks are now called with an an object of shape Update instead of Location

I wonder, how does that relate to the single change you committed

Sorry for typo, it should be location not listener. In v5, the first argument is not location, but an object of shape {location, action}. In that case, I extract .location property from it.

Using `!= null` is a bit confusing: it took me a moment to realise that this would also be true if the value is `undefined`, which (I presume) is the value we should test for. Using non-strict equal tests is often discouraged for such reasons.
@Treora
Copy link
Owner

Treora commented Jul 15, 2020

Sorry for typo, it should be location not listener. In v5, the first argument is not location, but an object of shape {location, action}. In that case, I extract .location property from it.

Ok, makes sense.

I made one tweak, as using != null is a bit confusing: it took me a moment to realise that this would also be true (I meant false, of course) if the value is undefined, which (I presume) is the value we should test for. Using non-strict equal tests is often discouraged for such reasons. Give a shout if we should actually test for null too.

@Treora Treora merged commit 742e892 into Treora:master Jul 15, 2020
@Hypnosphi Hypnosphi deleted the history-5 branch July 15, 2020 19:53
@Treora
Copy link
Owner

Treora commented Jul 16, 2020

Published on npmjs.com as [email protected]

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.

2 participants