Skip to content
This repository has been archived by the owner on Oct 11, 2022. It is now read-only.

Server-side rendering #1394

Merged
merged 36 commits into from
Sep 7, 2017
Merged

Server-side rendering #1394

merged 36 commits into from
Sep 7, 2017

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Aug 21, 2017

  • Make it possible to import the frontend routes from Node
  • Make it possible to render the app from Node
  • Implement react-router SSR
  • Implement styled-components SSR
  • Implement Apollo data fetching SSR
  • Implement react-helmet SSR
  • Client-side rehydration
  • Fix homepage
  • Fix meta tags for private stuff
  • Figure out ServiceWorker stuff
  • Fix rehydration (e.g. community page channels list disappears)
  • Fix react-helmet outputting default meta tags on the server
  • Sanitize initial state with https://github.com/yahoo/serialize-javascript
  • Fix thread opening

mxstbr added 2 commits August 21, 2017 09:22
To server-side render, we need to import our routes file in iris, i.e.
in a Node context. The issue is that in a Node context window is
undefined, navigator is undefined, etc. etc.

I removed everything that requires window access from the import path
when importing routes.js, while making sure the app still works as
expected.

This patch means we can `const App = require('../src/routes')` from Iris
which sets us up to do SSR!
These changes make it possible to create a Redux store on the server
with the server-side Apollo client and render our App in Node.
@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 21, 2017

I've got a first working version of SSR!

screen shot 2017-08-21 at 14 07 06

Notice the JavaScript disabled being ticked. Lots of things still missing but I think we'll be able to make it work.

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 24, 2017

Mind trying this locally @brianlovin?

# Install new deps
yarn
# Build a new version of the client
yarn run build:client
# Run iris, forcing it to serve the SSR'd client
DEV_SSR=true yarn run dev:iris

Now visit localhost:3001 with JavaScript disabled (DevTools -> three dots -> settings -> scroll to bottom -> disable javascript) and the ServiceWorker deleted (DevTools -> Application -> ServiceWorkers -> Unregister) and you should see the page even though no JavaScript runs! 🎉

Note: I'll likely end up turning off the ServiceWorker caching again because it doesn't make any sense with server-side rendering, still thinking about this.

@brianlovin
Copy link
Contributor

Working great!

@brianlovin
Copy link
Contributor

What do you need me to test? I've tried navigating to most pages and doing cold starts on most pages - seems to be working

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 25, 2017

Nothing else to test, just gotta iron out some last bits then this should be ready to go!

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 30, 2017

REMINDER FOR MYSELF

Reset the last commit 837fcd2 before merging in the changes from #1400 since I moved those over manually!

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 31, 2017

The new homepage on spectrum.chat on a fast 3G connection (throttled by DevTools) has the first meaningful paint after ~5-6s.

The old homepage with SSR on the same connection has the first meaningful paint after 1.2s.

😱 😱 😱

Note: The homepage is likely the only page anybody will see SSR'd, as after that the ServiceWorker kicks in and caches the bundle. The next time they visit the page they'll get the bundle served from their browser and they'll see the loading indicators immediately, no SSR happening there.

@mxstbr
Copy link
Contributor Author

mxstbr commented Aug 31, 2017

Based on the above numbers I want to ship this before I publish my blogpost. 😅 Keep pushing it out, but after this is shipped I'll publish it for sure!

Got through that ugly af merge with master, everything is still working. Now I gotta make sure the client-side rehydration is working as intended and not re-rendering, and then I have to implement some meta tags to make sure Google doesn't index /notifications et al. and theeeeen we might be good to go?

@mxstbr mxstbr changed the title WIP Server-side rendering PoC Server-side rendering Aug 31, 2017
@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

Sorry maybe I phrased that badly.

DO NOT USE localhost:3001 UNLESS YOU'RE WORKING ON SSR ITSELF

The only only only reason to use localhost:3001 is to develop SSR itself, otherwise always always stick with your current workflow and just use localhost:3000. Literally nothing changes from your current setup, nothing at all. Keep it as-is!

Would appreciate a follow-up commit to make that clearer in the docs if you want to rephrase it!

@brianlovin
Copy link
Contributor

Haha gotcha, will rephrase that :)

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

I fucked up the Babel merge, deploy didn't go through. Some or other plugin is missing, one minute...

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

NEW DEPLOY IS UP, TEST THE SHIT OUT OF THIS BABY THEN 🚢 https://spectrum-vcdymaxhmu.now.sh/

@brianlovin
Copy link
Contributor

Testing now.

Initial notes is that our thread fetching is very slow all of a sudden. Even when viewing a community page directly. But that's unrelated to this branch.

@brianlovin
Copy link
Contributor

Everything I can think to test is working great man!

@brianlovin
Copy link
Contributor

Before we deploy and alias can we piggyback #1431 and #1430 into the release?

@brianlovin
Copy link
Contributor

On /notifications we don't update the <title>:

screenshot 2017-09-07 12 13 49

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

Fixed meta tags in 761d1a5.

Note that it's very imperfect meta tags right now, I'll be doing another pass over those after we merge this probably. Let's shipit for now though.

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

I'm going to bed soon, feel free to deploy and alias all those together, but merge this one first!

@brianlovin
Copy link
Contributor

To be honest I'm a bit nervous to ship this immediately before you go to bed :P

@brianlovin
Copy link
Contributor

If we merge it now could you stay up for an hour or so just to make sure things work? Don't want to keep you up though, we can do it tomorrow if you want and I'll deploy those other two PRs separately

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

To be honest I'm a bit nervous to ship this immediately before you go to bed :P
If we merge it now could you stay up for an hour or so just to make sure things work?

Worst case scenario just revert to the last iris deploy; it's not like we're running irreversible migrations or anything! 😊 (seriously, you shouldn't feel blocked by me)

That being said, I'll be up for a couple of hours if shit really hits the fan just call me +436642324261.

@brianlovin
Copy link
Contributor

Alright. Let's do this shit.

brianlovin
brianlovin previously approved these changes Sep 7, 2017
@brianlovin
Copy link
Contributor

I think we accidentally merged in a deleted file. Removing quickly.

@brianlovin
Copy link
Contributor

Running locally yarn run dev:client returns this error:

screenshot 2017-09-07 12 46 07

@brianlovin
Copy link
Contributor

I've run yarn to get the latest packages, btw

@brianlovin
Copy link
Contributor

That error occurs because yarn run dev:client opens localhost:3000 - everything works just fine if I load localhost:3001, but I thought we weren't doing that?

@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

This is an upstream issue, it'll work on deploy: facebook/create-react-app#3041 (have to rm -rf node_modules locally)

@brianlovin
Copy link
Contributor

Great, merging everything in! Could you approve those other PRs?

@brianlovin brianlovin merged commit 60ae8c0 into master Sep 7, 2017
@brianlovin brianlovin deleted the sweet-sweet-seo branch September 7, 2017 19:52
@mxstbr
Copy link
Contributor Author

mxstbr commented Sep 7, 2017

YESSSS FINALLY!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants