Week7 ssr by Zaggen · Pull Request #16 · strvcom/react-nights-2019

Zaggen · 2019-04-29T17:11:38Z

Replaced create-react-app for next.js

prichodko

Thanks Lu 💪

I've found couple of little things, so just look for the comments. However there are two things outside of the codebase:

Rename public folder to static (and correctly implement handler of * requests), otherwise serving of static files won't work.
Maybe it's out of the scope of your lecture, but it would be pretty cool to show, that you can access cookies on the server and return page already with necessary data for authentication (e.g. no blink).

prichodko · 2019-04-29T21:15:08Z

+
+class CustomDocument extends Document {
+  render() {
+    const sheet = new ServerStyleSheet()


Although I can't really say, if there is difference or not, but according to official example and styled-components website code for example, styles extraction is defined in getInitialProps method.

No idea if it makes a difference either but I could change it just in case

prichodko · 2019-04-29T21:17:55Z

+    //server.use(compression())
+
+    server.get('/products/:id/:name', (req, res) => {
+      app.render(req, res, '/product', req.params)


Please add a return statement here, to indicate the end of the request.

prichodko · 2019-04-29T21:20:07Z

+    server.get('*', (req, res) => {
+      const parsedUrl = parse(req.url, true)
+      const { query, pathname } = parsedUrl
+      app.render(req, res, pathname, query)


For the handler of all incoming requests you can simply do

// at the top const app = next({ dev }) const handle = app.getRequestHandler() // and then server.get('*', (req, res) => { return handle(req, res) })

This is also necessary for the correct handling of static file serving.

Seems I removed that, I'll add it back

prichodko · 2019-04-29T21:21:56Z

-      </Layout>
-    )
-  }
+const ProductView = props => {


Let's move destructuring straight into arrow function declaration. 😊

prichodko · 2019-04-29T21:29:35Z

Another quite a big issue I see right now, is that you are not taking into consideration that @koss-lebedev during his lecture will move codebase to TypeScript.

dannytce

Overall great job! :) But I agree with Pavel. It would be cool to make it work somehow with Kosta's PR too :)

dannytce · 2019-04-30T08:45:42Z

@@ -0,0 +1,35 @@
+// @flow


Please remove it :)

Lol, will do

dannytce · 2019-04-30T08:46:07Z

@@ -0,0 +1,37 @@
+/* eslint-env node */


We don't need this either. It's already set in .eslintrc

dannytce · 2019-04-30T08:46:22Z

+/* eslint-env node */
+const express = require('express')
+const next = require('next')
+//const compression = require('compression')


Let's get rid off commented code :)

I was planning on uncommenting it on live demo, but I could type it directly.

dannytce · 2019-04-30T08:47:01Z

+      if (err) {
+        throw err
+      }
+      console.log('> Ready on http://localhost:' + PORT) // eslint-disable-line


Maybe let's be more strict -> eslint-disable-line no-console? :)

dannytce · 2019-04-30T08:47:53Z

@@ -1,15 +1,41 @@
+import fetch from 'isomorphic-fetch'
+// import config from '../config'


Why it's commented there? :)

dannytce · 2019-04-30T08:49:55Z

+const ProductView = props => {
+  const { product } = props
+  return (
+    <>


Fragmet not needed here

dannytce · 2019-04-30T08:52:10Z

-    <Link to={node.id}>
+    <Link
+      href={`/product?id=${node.id}`}
+      as={`${PRODUCT_LIST}/${node.id}/${kebabCase(node.name)}`}


Can we move this into routes.js to preserve previous logic?

I would have to rework that, as href is different than as, I didn't want to overcomplicate that logic, it might be easier to follow like this in my opinion.

dannytce · 2019-04-30T08:53:37Z


+ProductList.getInitialProps = async props => {
+  const products = await getProducts()
+  props.store.dispatch(loadProducts(products))


Since you are dispatching loadProduct, you don't need to have them in mapDispatchToProps, don't you?

I don't, good catch :)

dannytce · 2019-04-30T08:54:09Z

 import cart from './cart/reducer'
 import customer from './customer/reducer'

+const isBrowser = typeof window !== 'undefined'


There is is-browser helper, let's use it here :)

dannytce · 2019-04-30T08:54:45Z

@@ -0,0 +1,11 @@
+export const getRefreshToken = () => {
+  return window.localStorage.getItem('refreshtoken')


Shouldn't this be extended with isBrowser() && too?

Zaggen · 2019-04-30T09:20:40Z

@prichodko @dannytce Thanks for the feedback and @prichodko good catch with the static folder I completely forgot to change it, I would mention the sign-in via server but only for the account page as it would hinder caching if done on all routes.

Regarding kostas PR, we previously agreed on having this SSR as a separate branch as it would be overwhelming for the students if combined with typescript, but maybe something could be done

dannytce · 2019-05-03T14:09:54Z

Feedback implemented in #18

Zaggen added 30 commits April 29, 2019 18:02

Add next pages/routes

653dd02

Update dependencies

4d4a138

Add routes file

4484453

Add thunks

5b1a31e

Add utils/errors.js

cb97062

Add utils/is-browser.js

4c7569b

Adds utils/kebab-case.js

eae877a

Remove deprecated file

55b1a2f

Add server/index.js

2a71adb

Fast forward to new version of file

ff9ad46

Add api/customer/refresh-customer-token.js

d68c2ad

Removes /macro import from styled components

b3c9263

Refactor PrivateRoute into PrivatePage

98eb14a

Fast forward to new version of file

1619ecc

Updates Link to work with next

81c09ab

Add utils/refresh-token.js

0cc735c

Update dependencies and scripts

2d8cce4

Fast forward to newest version of the file

63b8fd4

Adds .next path to the ignored list

4f15d5b

Fast forward to newest version of the file

6dcc86b

Wraps in PrivatePage

fc4efc8

Adds pages/Logout

3c87e48

Fast forward to newest version of the file

06166a8

Change logic to support ssr

64a7dab

Adjust to support next/link

ef8ffd6

Fast forward to newest version of the file

873fc25

Fast forward to newest version of the file

7bb9415

Remove Layout

ec3f88e

Rework store to support server initialization

95f1418

Change logic to support SSR

5f1e753

Zaggen added 4 commits April 29, 2019 18:33

Add safety check for browser environment

556419e

Add @zeit/next-css

9ad0c0d

Set up withCss

250df6d

Add react-toastify styles

f0f38ed

Zaggen requested review from dannytce and koss-lebedev April 29, 2019 17:11

Zaggen changed the base branch from master to week5 April 29, 2019 17:18

prichodko suggested changes Apr 29, 2019

View reviewed changes

dannytce suggested changes Apr 30, 2019

View reviewed changes

Zaggen added 9 commits April 30, 2019 13:33

Rename public folder into static

5376599

Remove unused file

84e8166

Update fonts path

713a3b4

Minor refactor

ad5eec6

Remove flow tag

f7f4501

Add next's handle and return statements

ef85ab8

Adds isBrowser checks

e142b04

Import isBrowser to replace local implementation

a2d2876

Make eslint supperssion comment more explicit

aca64d8

dannytce closed this May 3, 2019

		@@ -1,15 +1,41 @@
		import fetch from 'isomorphic-fetch'
		// import config from '../config'

		@@ -0,0 +1,11 @@
		export const getRefreshToken = () => {
		return window.localStorage.getItem('refreshtoken')

Conversation

Zaggen commented Apr 29, 2019

Uh oh!

prichodko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prichodko Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prichodko commented Apr 29, 2019

Uh oh!

dannytce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaggen Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaggen commented Apr 30, 2019

Uh oh!

dannytce commented May 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prichodko Apr 29, 2019 •

edited

Loading

Zaggen Apr 30, 2019 •

edited

Loading