The inconsistency of Router between V1 and V2 #693

Closed
Huxpro opened this Issue Jan 7, 2017 · 13 comments

Projects

None yet

4 participants

@Huxpro
Contributor
Huxpro commented Jan 7, 2017

TL;DR. Next V2 will switch entire page instead of re-rendering current page when routeChange, which is inconsistent with Next V1.

I have elaborated this issue under zeit/nextgram#8.

@arunoda arunoda added the Type: Bug label Jan 8, 2017
@arunoda
Collaborator
arunoda commented Jan 8, 2017

This is something we need to investigate.
I'll use nextgram for this.

@arunoda arunoda self-assigned this Jan 8, 2017
@Huxpro
Contributor
Huxpro commented Jan 8, 2017

@arunoda Nice.

FWIW, I have forked a next@2.0.0 version of nextgram at https://github.com/Huxpro/nextgram/tree/next-2.0. Which have updated package.json and have migrated from next/css to styled-jsx, should reduce some time.

@arunoda
Collaborator
arunoda commented Jan 11, 2017

Okay. Here's what going on here.
On nextgram it uses a .push() method of props. That API's was wrong and we re-wrote it.

By going through the nextgram, it seems like we need to bring back the functionality of that API again. But, I don't know whether it's a good idea to bring it back?

/ cc @rauchg @nkzawa

Basically, here's what's happening in that API.

  • We change the URL of the route via push state
  • But it doesn't render the component assign to that URL
  • But render the current component
  • And pass query params of the current URL into that.

I smell bad for such an API.

Let's see how to fix this in a different way.

@arunoda
Collaborator
arunoda commented Jan 11, 2017 edited

Okay, here next 2 style version of nextgram.

Live: https://nextgram2.now.sh/
Repo : https://github.com/arunoda/nextgram
PR: zeit/nextgram#9

@nkzawa
Member
nkzawa commented Jan 11, 2017

@arunoda I think it should keep the original behavior which shows a modal over the list when you click a link, but doesn't show the list on SSR.

@arunoda
Collaborator
arunoda commented Jan 11, 2017

@nkzawa okay. That's possible. Let's do that.

@arunoda
Collaborator
arunoda commented Jan 11, 2017

Done.

@arunoda
Collaborator
arunoda commented Jan 11, 2017 edited

@Huxpro could you send me a PR with the updated styled-jsx syntax.

@rauchg
Contributor
rauchg commented Jan 11, 2017

@huxpro can you confirm the API now behaves as intended?

@arunoda arunoda removed their assignment Jan 13, 2017
@nkzawa
Member
nkzawa commented Jan 13, 2017

Please check zeit/nextgram#9

@nkzawa nkzawa closed this Jan 13, 2017
@Huxpro
Contributor
Huxpro commented Jan 13, 2017 edited

@nkzawa @arunoda @rauchg

Thx guys I found @nkzawa have updated style-jsx part. So basically @arunoda uses push(url, as=url) to achieve 'universal routing' without changing code in Next, kinda cheating on <Page> with a crafted url. How clever! Glad to see such flexibility.

so I just removed all react imports to make it more 'next smell' since react is a implementation details, and replaced imperatively router calls with <Link>. Please check out zeit/nextgram#11

BTW, I am still concerning about how to do nestedly or partially re-rendering between Page to Page in Next. Well, that is an enhancement and not related to this issue.

@rauchg
Contributor
rauchg commented Jan 13, 2017

Yep as is one of the neatest things out there. Massive simplification

@arunoda
Collaborator
arunoda commented Jan 14, 2017

BTW: I need to thank @nkzawa for showing me this simplest solution. I was using a custom server, but it's not needed in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment