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.

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

[WIP] RFC/Issue 115 Build Time Data Injection #251

Closed
wants to merge 19 commits into from

Conversation

hutchgrant
Copy link
Member

@hutchgrant hutchgrant commented Nov 4, 2019

Related Issue

#115

Summary of Changes

  1. Added apollo graphql server to be run before pages are serialized/compiled in development/production. Populated by our graph.

  2. Added graphql schemas for menus and items

  3. Modified all markdown pages to contain menu front-matter var

  4. Modified graph to include all menu front-matter for each page

  5. Added seperate plugin for graphql, although it's still part of the CLI for now

  6. Modified page-template.js to perform a graphql query for each menu based on the URL

Problems

1. Big problem is it isnt serializing in production. For now it's a work in progress that functions in development mode only.

  1. Second big problem is I need to prevent it from attempting to rehydrate the graphql query, as the server will not be accessible once built in production.

  2. The menu items are in alphabetical order(thats the way they're added to graph). This could be mitigated with numbered variables within front-matter and then sorted.

  3. The menu subitems aren't being parsed and are therefore absent. We could use remarkable to parse the markdown and add sub items to the graph whereas before we did it manually.

Notes

This is just an example of how graphql could be used to inject our graph or other data sources into our compilation

Plan

1. Get this small example to serialize in production by running line by line debugs of the serialization lifecycle
2. Get this small example to stop rehydrating after its already been serialized - partially working
3. Standardize and modularize then document.

@hutchgrant hutchgrant force-pushed the rfc/issue-115-build-time-data-injection branch from 0da875a to 560f303 Compare November 4, 2019 06:40
@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 5, 2019

Now we have working rehydration, except it's not completely working.

We are storing apollo's cache into memory and then reading it before any more queries are made. That's great, it works when we fetch a new page. However, when we fetch a page using our redux-router, we're not fetching a new page, instead we're rendering components conditionally based on a route within javascript. This causes a problem because our js bundle doesn't have our pre-rendered cache. It can however, retrieve the pre-rendered cache from the initial page fetch. But in our case, that would contain the wrong query, for the previous page that we've since navigated away from.

You can test this by navigating to an initial page, check the <script state="apollo"></script> child element within our page-template element. You'll notice that it does not change once you navigate to another route. e.g. click "about" and then "docs". As well you will notice the menu will not display on the second page(again, because of the routing loading the component instead of a new page). Refresh that second page and menu loads from the newly fetched page.

In development, it works as it should(because graphql is running on server). In production only when serving this you will see the rehydration issue.

@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 9, 2019

The solution here is that we need to rehydrate our queries, on route change, from static json files(as we have no graphql server in production for a static site), that we've already pre-rendered server side. We also need to rehydrate from the pre-serialized cache on the initial page fetch.

So what we're doing is, during build we're writing static json files to our public directory when a query is received. We're also serializing that query so that its available immediately within the html page when it's initially fetched by the client. When a client navigates to a new route, we're rehydrating by fetching the cache from a .json file we wrote earlier during build.

In this solution, the cache is in 2 parts. Once initially serialized as part of build. Second cache is written and fetch from file on route change.

Going forward, I'd like to refactor and modularize this so that this example can be extended for other applications without the large amount of code currently needed.

@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 9, 2019

A few problems,

1) it's not serializing the query properly now during build

2) initial page doesn't contain the cache after build

  1. we're fetching from cache for every single subroute change, which causes a noticable delay. This could be optimized, at least in this example, to only rehydrate on main category route change. Or do it in such a way where it simply changes the state instead of adding a new state to the page component's shadowRoot which causes the visible delay.

Logic within the page-template.js needs to be tweaked slightly. At least it's fetching the static cache, albeit potentially too often for this use case. These problems are noticable due to the slight delay before menu is rendered on initial page fetch.

@hutchgrant hutchgrant force-pushed the rfc/issue-115-build-time-data-injection branch from 760b67d to 9ae4db8 Compare November 9, 2019 07:18
@hutchgrant hutchgrant force-pushed the rfc/issue-115-build-time-data-injection branch from c8c9538 to bde6511 Compare November 9, 2019 08:29
@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 9, 2019

it needs even more work to the logic of page-template.js and eventually some form of graphql injection component.

During serialization, our query must pull from the graphql server at least once. After querying, add the graphql cache to the DOM within a script element so that its written to the final html file. This only needs to happen during build, so within our component we need to make sure the condition to do this is appropriate.

In production:

  1. we need to detect if a cached state is present on the initial html page fetch
    • if cached state, we need to rehydrate client with it before query. This gets problematic managing client state because we have to forcefully requestUpdate the page-template component after query
    • if no cached state exists, we need to fetch cache from our pre-rendered static json files then rehydrate cache before query

Doing all that conditionally is a royal pain in the arse.

@thescientist13 thescientist13 added the RFC Proposal and changes to workflows, architecture, APIs, etc label Nov 9, 2019
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Thanks for taking a pass at this, it's definitely a tricky challenge.

I do like the syntax that something like GraphQL brings to the table, but the implementation of which is a bit involved it seems. 😳

My initial thoughts on this were a bit more simplistic, I suppose, but I think it would work reasonably well enough and not have to "intrude" too much into user-land, so to speak. Given my snippet in the RFC description

import { graph } from 'xxx';

setupShelf() {
  // map over the graph and create the shelfList object dynamically
  graph.pages.map((page) => {
    // xxx
  } 
}

xxx would just be a JSON object (file or otherwise, e.g.)

{
  "graph": {
    "pages": [{
      "name": "home",
      "path": "/home"
    }, {
      "name": "about",
      "path": "/about"
    }]
  }
}

I was thinking of using something like webpack's resolve as the mechanism by which to do something like this for us. l'll plug away at this approach a little more will try and get a POC of it up in time for our next meeting. Being able to define a schema like GraphQL would be pretty awesome though, so I'll see if I can marry these two concepts. 🤞

(Maybe this could give rise to another use case for, or extension of, having webpack be able to input / output from memory?)

In the meantime, definitely continue on your work here and let's plan the bulk of our time during our next meeting to show and tell our approaches. ✌️

@thescientist13 thescientist13 self-assigned this Nov 13, 2019
@thescientist13
Copy link
Member

thescientist13 commented Nov 13, 2019

@hutchgrant
Not sure if this thread on hydration may be helpful here, but might be worth a read either way
https://twitter.com/rich_harris/status/1193986671764758529

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

So definitely some interesting stuff going on here, but I think introducing GraphQL may have been a bit premature.

In general, the workflow I had in mind was one where the user uses import from their code, and they get the graph. From there, they should be able to access all the data and build their own components / UI structures, e.g.

  • list of pages (e.g. blog posts)
  • navigation menus
  • <title> / <meta>

I think all the menu specific stuff I am seeing is a bit too specific at this point since it seems to have introduced a bit too much coupling / userland bleed than I would have expected, given the use cases defined above, e.g:

  1. menu in front matter
  2. menu in the graph
  3. queries

I wouldn't rule out allowing users to use things like map, filter, and reduce over the graph to derive the data they need.

Maybe we try an example without GraphQL, and just resolve import { graph } from '@greenwood/data' to that JSON file for now? I am not against using something like GraphQL, mostly for providing a nice DSL as a DAO, but it's not quite clear yet if we understand the right boundaries. I think it would be great to try something a bit simpler, even if it requires a little more boilerplate on the user side, it can show us where the pain points are and what is cumbersome, and then make sure we solve that problem correctly.

Maybe we could make some queries that would be more like "prepared statements" that we can provide for convenient / common things one might need from our graph, but I don't think we're at that level yet, and I think a couple steps away still from GraphQL. (with all the implications that brings, good or bad).

I think just building up the simplest implementation first can give us a valuable reference point to review and compare against the implementation here.

I would still recommend some clean up to this PR just make it a bit more easier to compare against other implementations, and so I would try and collapse as much of the code as possible under @greenwood/cli, in libs/, lifecycles/, etc since I think it would all need to be "core" code ultimately.

@@ -1,3 +1,8 @@
---
menu: about
Copy link
Member

Choose a reason for hiding this comment

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

making a note of userland bleed

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the simplest way to link a page directly with a menu.

Otherwise, you need to create seperate menu files, which we wanted to avoid to begin with, as we already do that. That would defeat the purpose of compiling those menus at build time.

Copy link
Member Author

Choose a reason for hiding this comment

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

#115 (comment) You also suggested this yourself

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah. Those weren't meant to part of the implementation / API, just metadata about the page itself that should be passed through (if possible) to the graph when pulling the graph data.

@@ -25,11 +25,15 @@
"@babel/core": "^7.6.0",
"@babel/preset-env": "^7.6.0",
"@webcomponents/webcomponentsjs": "^2.3.0",
"apollo-boost": "^0.4.4",
Copy link
Member

Choose a reason for hiding this comment

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

What's the relationship of all these dependencies. Do we need them all?

  • graphql
  • apollo
  • axios
  • cross fetch

Copy link
Member Author

@hutchgrant hutchgrant Nov 17, 2019

Choose a reason for hiding this comment

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

axios is for the ajax request, to get and hydrate from our server rendered cache on route change(when a route doesn't already contain the cache). cross fetch is a peer dependency required by one of the apollo libraries. graphql is of course a requirement of apollo.

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks. Will take a little more look into that side of things.

@hutchgrant
Copy link
Member Author

hutchgrant commented Nov 18, 2019

I've got it working, albeit its not initializing from cache properly on initial page load(it's fetching cache rather than using the serialized one). Of course I need to include the other submenus in the compilation and query, as well as add special conditions so that that the menus display properly on all the pages besides main navigation pages. But again menus is just one example.

We can build it in a modular way so any developer could add anything they want to the graphql server and query it via plugins etc. For now I'm only querying vars that I've added to each page's front-matter, again as an example.

Also page-template.js you can see how much code is required on the client to make it all happen smoothly. I'd like to create a custom element that handles this so all it involves is a query and result.

See lit-apollo for example

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Good chat tonight @hutchgrant !

To reiterate, this is definitely a great start on this work and I think provides a good foundation to build up our technical design from, in terms of what we both envision as being the simplest developer experience as we can achieve, as close to the examples we came up with.

Below are just some of my high-level thoughts coming out of my further review of this work and our discussion. Most of this I think we agreed with but just wanted to document it for posterity.

Connected Components

I think the promise of a lit-apollo as you found is very promising. I think if we can stuff as much of the boilerplate into app-template.js and let page templates or components just "connect" to the Apollo state would really nicely emulate the react-redux paradigm, which I always thought nice.

So combine that with gql and I think we're much closer to delivering a great workflow!

Derived State

To the point above, an important best practice with redux and stores in general, is try and keep data normalized and keep the store as simple as possible, which in our case, would be the graph. From there, components or templates can connect to get the subset of state they need, like for pages or subpages on their own.

I think the menu that was added to the markdown should be injected into the process in the same way that NodeJS would inject the path of the script when using __dirname, for example. In this way, we can promote the queries you have, but just leave out menu as anything the CLI / compilation needs to know about.

We (Greenwood) could provide a package of common queries for things like (just a thought)

  • nested menus (like in the shelf)
  • navigation bar (like in the header)
  • list of pages (like all pages/blog/*)

In the last example, but being able to pass through something arbitrary like this in markdown

date: 2019-11-19

Someone could then sort the blog posts on their own.

Misc

  • consolidate code into packages/cli
  • be more discriminating with our introduction of dependencies
  • we will need to start thinking about how / where to document this for users

I'll start tinkering with this now that i was able to get more insight and feedback from you. Getting there!!! 💪

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Thanks for all your good work on this @hutchgrant. 🙏 🎉

Closing this in favor of #269.

@hutchgrant hutchgrant deleted the rfc/issue-115-build-time-data-injection branch April 19, 2020 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Proposal and changes to workflows, architecture, APIs, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants