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

The inconsistency of Router between V1 and V2 #693

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

The inconsistency of Router between V1 and V2 #693

Huxpro opened this issue Jan 7, 2017 · 13 comments

Comments

@Huxpro
Copy link
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 vercel/nextgram#8.

@arunoda
Copy link
Contributor

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
Copy link
Contributor Author

Huxpro commented Jan 8, 2017

@arunoda Nice.

FWIW, I have forked a [email protected] 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
Copy link
Contributor

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
Copy link
Contributor

arunoda commented Jan 11, 2017

Okay, here next 2 style version of nextgram.

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

@nkzawa
Copy link
Contributor

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
Copy link
Contributor

arunoda commented Jan 11, 2017

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

@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

Done.

@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

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

@rauchg
Copy link
Member

rauchg commented Jan 11, 2017

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

@arunoda arunoda removed their assignment Jan 13, 2017
@nkzawa
Copy link
Contributor

nkzawa commented Jan 13, 2017

Please check vercel/nextgram#9

@nkzawa nkzawa closed this as completed Jan 13, 2017
@Huxpro
Copy link
Contributor Author

Huxpro commented Jan 13, 2017

@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 vercel/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
Copy link
Member

rauchg commented Jan 13, 2017

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

@arunoda
Copy link
Contributor

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.

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

No branches or pull requests

4 participants