Pre-rendered seo by hsab · Pull Request #46 · blerchin/dma-20-mfa-show

hsab · 2020-05-20T21:19:36Z

After a bit of testing with #45 I managed to get great results.
Decided to branch off of master again because I was just fiddling with code in #45. Wanted to make sure that the changes I've made are meaningful and appropriate. This PR is to be considered clean but there are still 2 issues left that I'm trying to solve.

Create React App: Title and Meta Tags
Create React App: Pre-Rendering into Static HTML Files
react-snap
react-helmet
SEO Recipe
react-snap Recipes

For production we just need to update this line to our final domain:

dma-20-mfa-show/src/Components/Seo/index.js

Line 7 in 25c71e8

const rootURL = `https://deploy-preview-46--kind-tesla-a088c1.netlify.app`;

Performance Metrics

Issues:

Incorrect class assignment to the blobs wrapper

Click on an artist.
Reload the artist page
Click on "Home" from the nav menu
Styling is incorrect. Assigned class is style_container__##XX where it should be blobs-wrapper

Lingering `","` at the end of `root` div

<div id="root">
    <div id="app">...</div>
    ","
</div>

hsab

Some comments and explanations on the code

hsab · 2020-05-20T21:21:04Z

 import paper from "paper";

-import style from "./style.module.scss";
+import './style-blobs.css';


This managed to fix the styling when the user enters the homepage from and external link or by typing in the address bar.

very strange that this would happen here and not break the other components that use css modules.

Yeah exactly my thought.

I can't reproduce this. I renamed style-blobs.css to style.module.css, removed blobs- from the style names. and then

import { wrapper, popover, } from './style.module.css';

in the index.js and then className={wrapper} etc and the correct styles are being applied. It does appear as style_wrapper__1dMCn but that's just the generated name from the css modules. The content of the class is accurate.

or was this only happening on Production? Just realized that

This is happening in production. Also I have temporarily fixed it by removing the class assignment on PageHeader. Have a look at the last commit when you can 7767417

hsab · 2020-05-20T21:21:45Z

  return (
    <div
-      className={style.wrapper}
+      className='blobs-wrapper'


Again fixing styling when entering the homepage from an external link or by typing in the address bar. #46 (comment)

hsab · 2020-05-20T21:21:59Z

+      className='blobs-wrapper'
      ref={wrapperEl}
-      style={collapsed ? {} : { width: parentWidth, height: parentHeight }}
+      // style={collapsed ? {} : { width: parentWidth, height: parentHeight }}


#46 (comment)

hsab · 2020-05-20T21:22:08Z

        </div>
      )}
-      <div className={style.popover} style={popoverStyle}>
+      <div className='blobs-popover' style={popoverStyle}>


#46 (comment)

hsab · 2020-05-20T21:22:31Z

@@ -1,22 +1,22 @@
-.wrapper {
+.blobs-wrapper {


Changing from SASS to pure CSS #46 (comment)

did you try this with the modules in place? I'm wondering if it was actually sass that was breaking it.

I'm gonna give it a try and report back.

No success :(

I found:

Reload applies adds CSS Classes to a div that shouldn't have it stereobooster/react-snap#437 which seems to be exactly our issue as "this only occurs when running the production build".

weird element placement behavior stereobooster/react-snap#214 (comment)

Dynamic CSS theme and react-snap stereobooster/react-snap#432

material-ui / jss / dynamic imports stereobooster/react-snap#99 (window.snapSaveState?)

There's also the following in my console during dev preview:

Warning: Expected server HTML to contain a matching <div> in <div>. in div (at App/index.js:10) in Router (created by BrowserRouter) in BrowserRouter (at App/index.js:9) in Unknown (at src/index.js:17) in StrictMode (at src/index.js:16) console.<computed> @ index.js:1

Wondering if this has to do with that lingering "," but figuring it out is way over my head and my react knowledge.

ok. i can take a look after the lecture tonight

Hey managed to narrow down the issue to PageHeader component's outermost div. Removing the className on this element fixes the problem. But no matter how hard I tried to add the class back, be it through pure css, having the css in the public folder and linking in html, sass, etc, it kept messing up the bg titles after returning to homepage from an artist page. I hope this helps.

7767417

hsab · 2020-05-20T21:22:59Z

+      category,
+      tags,
+      twitter,
+      image: image ? image.images[image.images.length - 1].path : 'FALLBACKIMAGE'


FALLBACKIMAGE needs to be changed to default image later on.

hsab · 2020-05-20T21:23:49Z

              return (
                <Route path={`/${slug}`} key={slug}>
-                  <Comp />
+                  <Comp config={config.artists[i]}/>


Passing the config data to the artist component to automatically infer the path and title.

i'm not sure if this will do what is intended. Lazy is used to code split the bundle, so each artist has their own js file instead of loading them all at once. Or is this just to pass the SEO metadata?

This is to just pass the SEO data from the config to the component. I'm not sure if what I'm doing is right. But the styling issue existed even when I had the metadata hardcoded with this file unmodified.

weird. lets see how removing sass goes and then i can poke around if you want. If the site is going to be static though you could remove the lazy stuff entirely since it won't help too much.

It seems like snap supports webpack chunks. Is this what Lazy uses? Based on what I know snap is only semi-static and still depends on being hydrated, generating the most basic html needed for pre-rendering. Wonder if chunks help in this regard. Also glancing over the generated artist htmls I see a lot of xxxxx.chunk.js

hsab · 2020-05-20T21:25:04Z

  font-family: sans-serif;
  font-weight: 700;
  line-height: initial;
+  margin-top: 50vh;


Again fixing styling when entering the homepage from and external link or by typing in the address bar. Translation is to accommodate centering in the absence of
style={collapsed ? {} : { width: parentWidth, height: parentHeight }}
#46 (comment)

… but this was my first stab and it is slightly cleaner) adds blob style modules back blobfield checks for collapsed state via useLocation rather than route callback reorder blobfield to be before switch adds wrapper class back to header removed lazy and suspense (this gave me about 20 more FPS on the blobs!)

brysonian · 2020-05-21T06:31:30Z

Ok just pushed a bunch and i believe it is fixed. It seemed to be something up with the router. So i shuffled some things there, the main fix was putting blob field at the top and adding useLocation to set the value of collapsed vs a route with a match callback. Seems to all be working now although i can't say 100% that i understand why, which is frustrating.

As a side effect, i removed the code splitting to see if that was causing trouble and got a remarkable increase in speed from that? Possibly some heisenbuggy non-sense, but it was reproducible, and the load trade off is minimal since the chunks are actually much smaller than i anticipated.

No luck on that freaking comma, need to not be on the computer for a while but i'll look again later tonight or more likely in the monring.

brysonian · 2020-05-21T06:46:10Z

It's always the little things lol 05cb69e#diff-1fdf421c05c1140f6d71444ea2b27638R17

lmccart · 2020-05-21T16:05:00Z

erp, my bad with the single quotes. now I know!

brysonian · 2020-05-21T16:11:13Z

Oh no i was linking to that one line #17, there was a comma in there. The single quotes change i think was hirad and is just personal preference.

lmccart · 2020-05-21T16:12:55Z

ohh haha I didn't really know how single vs double quotes could result in a comma but the internet never ceases to amaze me

hsab · 2020-05-21T19:48:25Z

It's always the little things lol 05cb69e#diff-1fdf421c05c1140f6d71444ea2b27638R17

oh my. that was a leftover from the argument separator to render. Totally missed that one.
Thank you so much for looking into this Chandler and fixing the class assignment issues! I really appreciate it <3

…auses some issue when directly visiting the homepage

blerchin · 2020-05-21T20:15:35Z

did y'all ever figure out the style issue? if it's only happening on production it probably has something to do with MiniCssExtractPlugin. webpack uses style-loader in development, which injects css via a javascript file and facilitates hot-reloading. In production we'd rather use actual css files so the ExtractPlugin generates flat css files.

hsab · 2020-05-21T20:17:10Z

@blerchin yeah chandler managed to fix it. Not sure if we figured out the cause though
5f2e343

hsab · 2020-05-21T20:19:20Z

@blerchin @brysonian This looks good to be merged into master.
For production we just need to update this line to our final domain:

dma-20-mfa-show/src/Components/Seo/index.js

Line 7 in 25c71e8

const rootURL = `https://deploy-preview-46--kind-tesla-a088c1.netlify.app`;

Also fixed the seo/smo stuff for homepage in the last commit

blerchin · 2020-05-21T20:20:00Z

As a side effect, i removed the code splitting to see if that was causing trouble and got a remarkable increase in speed from that? Possibly some heisenbuggy non-sense, but it was reproducible, and the load trade off is minimal since the chunks are actually much smaller than i anticipated.

Fascinating. With code splitting webpack has to duplicate a lot of dependencies so that may have been a source of slowness. Also with static imports it can do some additional optimizations, tree-shaking and stuff. So that might be speeding things up and making the single bundle smaller than expected.

brysonian · 2020-05-21T20:21:54Z

re: classnames, i was something in router combined with the hydration. but to find the exact problem would require diving into the guts of those libs

blerchin · 2020-05-21T20:23:22Z

@hsab still getting up to speed with this. I will take a quick look to try to familiarize myself. But excited to have this working!

We may need to remove public/_redirects which causes netlify to always load index.html no matter what file is requested.... Actually I'm surprised the seo is working at all with that in place

hsab · 2020-05-21T20:25:48Z

@blerchin
curl https://deploy-preview-46--kind-tesla-a088c1.netlify.app/graham-akins
Shows the proper static index for artist being loaded.

hsab · 2020-05-21T20:29:51Z

Also if this of any help, you can see the redirections:
https://developers.facebook.com/tools/debug/?q=https%3A%2F%2Fdeploy-preview-46--kind-tesla-a088c1.netlify.app%2Fhirad-sab

blerchin · 2020-05-21T20:31:53Z

weird stuff. Well, it's still working without _redirects. In any case, it's good not to depend on proprietary config stuff.

blerchin · 2020-05-21T20:37:39Z

I looked over the diff, and while I don't have a lot of context on this it all LGTM.

hsab · 2020-05-21T20:39:28Z

I looked over the diff, and while I don't have a lot of context on this it all LGTM.

If you're going to approve, let me update the link to our master preview.

…w into pre-rendered-seo

blerchin · 2020-05-21T20:40:53Z

@hsab when you feel it's ready, you can approve and I'll merge 😏

hsab · 2020-05-21T20:42:40Z

@blerchin Should be good now. I can't approve since I'm the PR author.

blerchin · 2020-05-21T20:43:00Z

@hsab oh, I didn't realize that's how it works. I'll approve in that case

hsab · 2020-05-21T20:57:07Z

yaaaaaay!

hsab added 2 commits May 20, 2020 13:55

doing a clean edit off of master

05cb69e

update preview url

7aca5d7

hsab commented May 20, 2020

View reviewed changes

hsab and others added 2 commits May 20, 2020 16:19

narrow down the issue

7767417

remove comma after app

35e0e93

hsab added 2 commits May 21, 2020 12:58

remove assigning wid/hei based on window. this is not necessary and c…

c1b9b4d

…auses some issue when directly visiting the homepage

add seo to homepage

25c71e8

experiment without _redirects

05a1d29

hsab added 2 commits May 21, 2020 13:40

update seo url to netlify master preview

61d2090

Merge branch 'pre-rendered-seo' of github.com:blerchin/dma-20-mfa-sho…

481c551

…w into pre-rendered-seo

blerchin approved these changes May 21, 2020

View reviewed changes

hsab merged commit d741968 into master May 21, 2020

		@@ -1,22 +1,22 @@
		.wrapper {
		.blobs-wrapper {

Conversation

hsab commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

Incorrect class assignment to the blobs wrapper

Lingering "," at the end of root div

Uh oh!

hsab 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!

hsab May 20, 2020 • 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!

hsab May 20, 2020 • 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!

hsab May 20, 2020 • 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!

hsab May 20, 2020 • 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!

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!

hsab May 20, 2020 • 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!

brysonian commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brysonian commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmccart commented May 21, 2020

Uh oh!

brysonian commented May 21, 2020

Uh oh!

lmccart commented May 21, 2020

Uh oh!

hsab commented May 21, 2020

Uh oh!

blerchin commented May 21, 2020

Uh oh!

hsab commented May 20, 2020 •

edited

Loading

Lingering `","` at the end of `root` div

hsab May 20, 2020 •

edited

Loading

hsab May 20, 2020 •

edited

Loading

hsab May 20, 2020 •

edited

Loading

hsab May 20, 2020 •

edited

Loading

hsab May 20, 2020 •

edited

Loading

brysonian commented May 21, 2020 •

edited

Loading

brysonian commented May 21, 2020 •

edited

Loading

hsab commented May 21, 2020 •

edited

Loading

hsab commented May 21, 2020 •

edited

Loading