Skip to content

chore(rebuild): properly utilize static html #2074

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

Merged
merged 7 commits into from
May 6, 2018

Conversation

jeremenichelli
Copy link
Member

Initially this PR solves a problem that the html files were generated with [object Object] content.

That said, we have anothr problem, and it's a BIG one because I know how hard it is solve the server side vs. async content loading issue in React.

Let me show you why. Right now we are creating the pages so they are static and already gather the content for it.

When the bundle hits the browser and runs, it detects the Promise and cleans the content state whiping out the server side generated content and remounting it again.

kapture 2018-04-24 at 14 30 57

This is serious because if we don't solve it we might need to move to full SPA which for a documentation site I don't recommend, or move to another static generator like Gatsby, Next or Nuxt.

@jeremenichelli jeremenichelli changed the base branch from master to rebuild April 24, 2018 11:45
@EugeneHlushko
Copy link
Member

Hydrate should do, i will try to take a look tonight at this

@jeremenichelli
Copy link
Member Author

jeremenichelli commented Apr 24, 2018 via email

@skipjack
Copy link
Collaborator

Yeah and using remark-react may help as well. The only problem with that is image resolving/loading but we could solve that with another remark plugin. I’m at a conf this week so pretty busy but I’ll get back to reviewing/merging/helping this weekend.

@jeremenichelli
Copy link
Member Author

I'm completely burnt out around this one, will see if I can tackle on the weekend and with a cleared mind something pops up.

@skipjack
Copy link
Collaborator

@jeremenichelli sorry for the delay on this and I see your point. I think there's basically two problems, but the first is the biggest:

  • There's no way to delay the render of Page after rendering the rest of app (w/o causing the content to go blank.
  • Using dangerouslySetInnerHTML may always cause a slight flash because it doesn't leverage React's diffing algorithm. Using remark-react would only fix this part.

The only thing I can think of to solve the first issue is moving all routing to the top-level and mounting the router prior to the rest of the application. By doing this, we could trigger a separate render / hydrate that would populate the site only once the bundle has loaded. This might, and probably will, have some other implications though. Maybe there's a way we could do something similar only during initialization since that's the only time we need it to be delayed.

If this doesn't seem like a good approach then maybe dropping the DirectoryTreeWebpackPlugin and direct use of the SSGPlugin and moving to Gatsby is the right choice.

@jeremenichelli
Copy link
Member Author

@skipjack though I should look at remark and see what it doesn't, I have the feeling that is not going to solve our issue here. I will give it one more try between today and tomorrow to see if I can avoid this, I really want to make sure we did everything we can before jumping into other solutions like Next, nuxt.js or Gatsby.

@jeremenichelli
Copy link
Member Author

Either way, this fix is necessary for properly render static pages and hydrate the DOM, so if it looks good, approave it so we can merge it and take this as base for future improvements.

@skipjack
Copy link
Collaborator

@jeremenichelli yeah I wouldn't worry too much about remark and remark-react yet as that's not the main issue. This PR looks good to me but I haven't tested yet so I'll do that soon.

I think I have an idea of how to fix the first (and bigger) issue though. We should be able to pre-load that first bundle in src/index.jsx and pass that down so there's no flash (i.e. "loading...") on the first render. Meaning, do one top-level import() so routing can remain in the Site and be utilized by both index.jsx and server.jsx. I'll try this out when I pull and test this branch and let you know what happens. If it works, I'll separate the fix as another PR.

@EugeneHlushko
Copy link
Member

Sorry for late reply, going to pull this and test tomorrow

@jeremenichelli
Copy link
Member Author

I'll see if I can avoid hydrate from remounting the whole component too.

Someone form us should have a solution, I suggest branching from this PR.

@skipjack skipjack changed the title Rebuild static html fix chore(rebuild): properly utilize static html May 2, 2018
skipjack added 6 commits May 6, 2018 19:18
This change delays the dynamic part of app from mounting until the first page's
content has loaded. This prevents the static content from being stripped from the
DOM until dynamic bundle is there to replace it. Once we migrate to `remark-react`,
the elements won't even be re-mounted as React's diffing algorithm will be used.

All other pages are still loaded using dynamic `import()`s.
Copy link
Collaborator

@skipjack skipjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything mentioned should be resolved now between @jeremenichelli's initial work and #2108 which I just merged into here. We could further improve improve rendering via remark-react which would leverage virtual dom diffing (where as dangerouslySetInnerHTML does not). Merging this to master and starting to work on merging some of the other rebuild related PRs as well.

@skipjack skipjack merged commit 1bb7852 into rebuild May 6, 2018
@skipjack skipjack deleted the rebuild-static-html-fix branch May 6, 2018 23:24
@montogeek
Copy link
Member

Great work @skipjack !

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

Successfully merging this pull request may close these issues.

4 participants