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.

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

Jump to bottom

New preset with hybrid SSG/SSR (replaces SSR-only with hybrid rendering modes support) #42

Closed
wants to merge 203 commits into from
Closed

New preset with hybrid SSG/SSR (replaces SSR-only with hybrid rendering modes support) #42

wants to merge 203 commits into from

Conversation

Vadorequest
Copy link
Member

@Vadorequest Vadorequest commented Apr 26, 2020 •
edited
Loading

Closed and moved to #68 because we renamed the preset branch.

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit cee3e6e failed to deploy to https://nrn-v1-ssr-mst-aptd-gcms-lcz-sty-c1-fcomhzhq5.now.sh (click to see logs)

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit 58bca45 failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-kkdptri5v.now.sh (click to see logs)

@Vadorequest Vadorequest mentioned this pull request Apr 26, 2020
Closed
@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit f747475 failed to deploy to (click to see logs)

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit abefe61 failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-122y3x676.now.sh (click to see logs)

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit f309835 failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-lm8ws9bed.now.sh (click to see logs)

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit 891a590 failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-f0oe6qb58.now.sh (click to see logs)

@Vadorequest
Copy link
Member Author

Vadorequest commented Apr 26, 2020 •
edited
Loading

@samuelcastro FYI SSG works with the Index page now. See https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-122y3x676.now.sh/. Logs at http://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-f0oe6qb58.now.sh/_logs

image

There are still some issues.

It's just a POC with SSG at this time. One interesting thing is that it's not required to remove the _app:getInitialProps implementation, because SSG overrides it.

@samuelcastro
Copy link
Contributor

this is great @Vadorequest, thanks!

@samuelcastro
Copy link
Contributor

CI is failing though, are you still working on it?

@Vadorequest
Copy link
Member Author

CI is failing but it's unrelated with the PR, someone's working on it.

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit f9259c2 failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-dh5hiyy9j.now.sh (click to see logs)

@samuelcastro
Copy link
Contributor

Any thoughts why language change is not working properly?

@Vadorequest
Copy link
Member Author

Nope, but I think I need to re-think much of the current implementation related to cookies

There is much complexity to support both SSR/SSG with advanced features such as i18n, it's gonna take me another week, at least.

@samuelcastro
Copy link
Contributor

Make sense.

@samuelcastro
Copy link
Contributor

samuelcastro commented Apr 29, 2020 •
edited
Loading

Why are you using getStaticProps over getServerSideProps? Don't we need to fetch languages on each new page request?

@Vadorequest
Copy link
Member Author

Vadorequest commented Apr 30, 2020 •
edited
Loading

Interesting, I hadn't thought of that. But I don't believe it's the best way to go for SSG, because it's not really SSG.

Here is the workflow/behaviour I intend to implement (for i18n SSG/SSR support), after reading through many discussions (i.e: vercel/next.js#10651)

  • No client-side language detection.
  • Use the language provided in the url path (e.g: /fr/, /fr/terms)
  • For the cases where the language is not provided in the url path (e.g: /terms) then redirect to an API endpoint (e.g: /api/redirectLocalePage) which detects the browser locale (based on the request HTTP headers) and then redirects to the desired page (see https://codesandbox.io/s/nextjs-i18n-staticprops-92ebn?file=/next.config.js)
  • Thus, I'll remove current implementation of _app.getInitialProps and _app.render
    • This will avoid confusion as to what is SSR/SSG (e.g: Next.js warning when using _app.getInitialProps wrongfully stating that we opted-out of SSG support)
    • It'll avoid execution of code in _app.getInitialProps when generating SSG pages at build time (see getStaticProps on _app vercel/next.js#10949 (reply in thread)), which may lead to errors depending on the use case
    • At some point in the future, it may be possible to use _app.getStaticProps for consistent behaviour without duplicating code (see getStaticProps on _app vercel/next.js#10949 (comment)), but for now only _app.getInitialProps is available, and until then I prefer to get rid of it to avoid any confusion
    • Basically, the _app will only contain error-related stuff (componentDidCatch)
  • I'll need to find a different implementation for cookies/userSession, probably using HOC or context
  • The Layout implementation will drastically change, because the current way is too limited if you need to use different layouts (per-page basis), and since we'll remove all code from _app.render, it'll become the job of the layout to init most of the context and variables that are currently defined in _app
  • The loading of the locales (Locize) must only happen once, it'll probably be handled by the Layout
  • The implementation of analytics (Amplitude) will probably have to be done through a HOC/useState, and handled by the layout
  • The init of the GraphQL (Apollo) will probably have to be managed by the Layout (done by HOC on _app currently) - _Not sure about that, maybe that'll stay on the app level 🤔
  • Preview mode for dev/staging environment, will be useful for the SSG page that load data from GraphCMS (e.g: /examples), I'm not sure if header/footer component will use preview mode at this point, or if it'll be pure SSG

@samuelcastro As you can see, there are a lot of changes to cover, and it's non-trivial. I wonder what you think about this design, do you see any flows?

@Vadorequest Vadorequest mentioned this pull request Apr 30, 2020
Merged
@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment FAILED
Commit 4b8900f failed to deploy to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-3elautm3q.now.sh (click to see logs)

@Vadorequest
Remove existing index/terms pages (SSR) and use experimental redirect…
9eacc5f 
… when language is missing (hardcoded to /en)
This was referenced May 26, 2020
Closed
Closed
@samuelcastro
Copy link
Contributor

FYI: #64

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit 555b44b successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-i6tj62ina.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest
Copy link
Member Author

Huge PR #52 has been merged, it's basically a Demo v2.

Demo at https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-i6tj62ina.now.sh/fr

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit ec66fef successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-jks83lkr8.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit 620c8c9 successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-ofrc27cuo.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest Vadorequest added the preset-branch This branch is a preset and is not meant to be merged, but to be used as a DIFF tool for learning. label May 28, 2020
@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit 916e2a2 successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-peyfrlyfe.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit d46366f successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-boopmcju8.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest
Misc reorder SSG pages (getStaticPaths before getStaticProps) for con…
0d0714c 
…sistency with Next.js own example (and makes more sense from a dev standpoint too)
@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit 0d0714c successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-6pjn4fhjc.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest
Copy link
Member Author

[GitHub Actions]
Deployment SUCCESS
Commit e370bf5 successfully deployed to https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-ao0l3r888.now.sh
Deployment aliased as https://v1-hyb-mst-aptd-gcms-lcz-sty.now.sh

@Vadorequest
Copy link
Member Author

[GitHub Actions]
E2E tests SUCCESS

@Vadorequest Vadorequest mentioned this pull request May 30, 2020
Open
@Vadorequest
Copy link
Member Author

Closed and moved to #68 because we renamed the preset branch.

The documentation has been updated as well, and we are now on NRN v2.
Thanks to everyone who helped with this release!

I'll try to make it easier to follow what new presets are being considered/planned in a short future.

@Vadorequest Vadorequest deleted the v1-hyb-mst-aptd-gcms-lcz-sty branch May 30, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7iomka 7iomka 7iomka left review comments

@ludovicmnji ludovicmnji ludovicmnji left review comments

@samuelcastro samuelcastro samuelcastro left review comments

@mxck mxck mxck left review comments

Assignees
No one assigned
Labels
preset-branch This branch is a preset and is not meant to be merged, but to be used as a DIFF tool for learning.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Vadorequest @samuelcastro @adrai @ludovicmnji @mxck @7iomka @Demmonius