Rfc/issue 115 build time data access

[thescientist13]

Related Issue

resolves #115

Summary of Changes

takes deep breath...

First of all, BIG shout out to @hutchgrant for the getting this going in #251, of which this PR was based off of. Will try and highlight all the relevant additions / modifications here, but there is a lot to go through so will do my best. Still a couple more TODO items and questions to go over, but this is it. We did it. 🏆

@greenwood/cli/data

Created this "alias" package to expose server, client, and cache.js files for the CLI and for consumers.

Main thing of note is that we wrap ApolloClient and determine where the data should come from.

Note: Bear in mind, this is being naively done. Should find a way to do it without the .catch handler

Server Side Rendering

One difference is now the data is being rendered server side. As the calls come into the GraphQL server, they will be written to uniquely-hashed named JSON files, and then during the serialize phase are written to index.html in window.___APOLLO_STATE. This implements a more traditional server side rendering technique and integrated it with the header

For example, while the code for the header is

<nav>
  <ul>
    ${navigation.map((item) => {
      return html`
        <li><a href="${item.link}" title="Click to visit the ${item.label} page">${item.label}</a></li>
      `;
    })}
  </ul>
</nav>

In the serialized HTML, we now see this! (which wasn't happening when it was being client side rendered)

<nav class="style-scope eve-header">
  <ul class="style-scope eve-header">
    <!---->
        <li class="style-scope eve-header"><a class="style-scope eve-header" href="/about/" title="Click to visit the About page"><!---->About<!----></a></li>
      <!---->
        <li class="style-scope eve-header"><a class="style-scope eve-header" href="/docs/" title="Click to visit the Docs page"><!---->Docs<!----></a></li>
      <!---->
        <li class="style-scope eve-header"><a class="style-scope eve-header" href="/getting-started/" title="Click to visit the Getting Started page"><!---->Getting Started<!----></a></li>
      <!---->
        <li class="style-scope eve-header"><a class="style-scope eve-header" href="/plugins/" title="Click to visit the Plugins page"><!---->Plugins<!----></a></li>
      <!---->
  </ul>
</nav>

The client the user consumes will be able to intelligently tell if the calls should be live (development mode, or read from cache / disk instead of over the wire)

GraphQL

To start things off, I have defined the CLI's graph as a type in GraphQL. I am using this to render the header and confirming that the Shelf would correctly get the data, but there is the issue of sorting though...

We are lucky that the navigation order in our header is already alpha-sorted. I think we will need to improve how we can query the graph in a more "declarative" way and / or customizable from a user perspective (e.g. providing a custom sort order). Will discuss more in the Follow Ups section.

I left console logging in for now to help with seeing this all working. Will remove before the PR is merged.

TODOs

1. Production console.log from the shelf not display response data (though cache.json file is loading fine)
2. Figure out a better way to detect "offline" mode other than fetch().catch()
3. "Fully" define Page typeDef defintion based on internal graph object
4. Add test cases for graphql (.json files, serialized output)
5. Review docs, add links to examples in our own code?
6. Review package.json dependencies, cleanup where applicable
   • using deepmerge since apparently deep merge is not a one-liner yet? 1, 2
7. Confirm that not closing GraphQL server during serialize lifecycle isn't leaving behind a zombie process
8. Review / address outstanding TODOs in the code
9. Too much overlap with "cli" test? e.g. state.getAttribute('data-state')
10. unit tests for resolvers (nice to have, make a ticket) - add unit tests for resolvers / schema #280
11. Have this be the start of the release/0.5.0 branch and PR, tracking relevant issues

Follow Ups

1. Make an issue to model Greenwood's config as part of the GraphQL schema (this would address <title> and ) (as part of 0.5.0 release). This will also need to figure out a way to merge resolvers / typeDefs in schema.js. - Made expose Greenwood configuration as data #270
2. Support "declarative" queries, or some sort of sorting, so resolver's can be more flexible then returning everything in alphabetical order and thus not fixed to the filesystem. (as part of 0.5.0 release) - query results are not deterministic #271
3. There are implicit contracts that could be very much improved here. Would be good to track individually:
   • That the graphql server writes to the cache - which has to make a call back to the graphql server just to extract the data... that's definitely an opportunity to optimize improve cache.json generation implementation (reduce duplicated grapqhl calls in serialization workflow) #272
   • In resolvers, assuming a certain amount of depth to the pages, e.g. split('/')[0] - resolvers and file writing assumes certain route depth #273
   • generating JSON in cache, globbing in serialize, assuming filename cache.json - improve performance and generation of cache.json file #277
4. Make an issue to implement passing through arbitrary front matter data to the graph (like a date for a blog post) - support passing front matter from pages to graph (custom content metadata in markdown)  #274
5. Should data access be part of the Getting Start guide? - yes, but as branch. Use the docs data page to demonstrate the graph.
6. remove require calls from the shelf - add support for dynamic imports  #275
7. Make an issue for tracking Browserslist: caniuse-lite is outdated. Please run next command yarn upgrade - update browserslist version #276
8. Can we hot reload the graphql server like webpack-dev-server? - added to Trello for review / tracking after 0.5.0 release
9. Need to confirm if we have sourcemaps implemented... - added to Trello for review / tracking after 0.5.0 release
10. @greenwood/cli/data as its own package? - GraphQL (Apollo) data as its own package #278

[thescientist13]

had to do this to keep the browserslist message from interfering in our tests that were testing for stderr
browserslist/browserslist#361

[hutchgrant]

I see that in development mode as well.

Browserslist: caniuse-lite is outdated. Please run next command yarn upgrade``

[thescientist13]

Ok, got a good amount of test cases in now for data serialization, however, and I think this may more have to do with usage of JSON.stringify / JSON.parse not maintaining consistent ordering or keys (even when trying to use deep.equalInAnyOrder in the tests), but it further highlights the need why something like #271 needs to be implemented as part of any release that includes this RFC.

Essentially, if you look at the test for Blog Posts, because things aren't deterministic, the order of blog pages (/blog/first-post/ and /blog/second-post/) can come back in inconsistent order which is why I can't test the full title / link contents, and had to fallback to using .contain as opposed to .equal. In fact, while equalInAnyOrder makes testing easier, think of a user getting a different order of results potentially each time. Probably gonna have a bad time. ⛷

So not only should be users be able to specify an order if they want, but at the very least our default return order should always be consistent. I will include a comment to this effect in #271 and escalate its priority.

At least it's enough to finish the last few TODOs on my list though, which I will be able to clean up tonight.

[thescientist13]

@hutchgrant / @aholtzman
OK! I think this is ready to go now, please let me know if you have any feedback. I will plan on using this as the start of a release/0.5.0 branch with the set of issues to be included per our last meeting.

Phew, really excited to land this one! ✈️ 💪

[hutchgrant]

I'm happy to see these changes coming and especially glad to see large improvements made on my initial idea to something much more stable, practical and polished. The rehydration issue with the client and the render issue of the cache on the server being fixed made me excited. I watched every improvement to this PR and am glad we're moving this forward to 0.5.0

On initial cursory review, I found that the sorting issue being fixed is a necessity for successful testing and in practise. Perhaps that can be added post-merge. As well I found tiny things you missed in your continued improvements(typo here, outdated info there)

I would like the cache reading/writing loop to be done asynchronously pre-merge with 0.5.0 release branch as I feel it shouldn't require that many changes and is necessary.

I'd like to go over the documentation first, along with these tiny changes, before I give full approval, but you already know how I feel about this 🎉

edit: your apollo wrapper(inspired by lit-apollo no doubt) solution, to handle the conditional rendering/fetching of the cache, is good, simple, logic 🥇

[hutchgrant]

I'm concerned this section needs to be asynchronous. Reading a file, merging a file, writing to file, and repeating that through however many pages. That's going to cause problems if not done asynchronously. For an RFC it's fine, but before release, that needs to be fixed. #277

[hutchgrant]

'@greenwood/cli/data/queries/cnildren';

typo: /children

[hutchgrant]

I realize console.log is for debug of shelf and can be removed later I suppose with #275

[thescientist13]

yeah, we'll definitely want to catch. I did notice that in prod mode (at least) the shelf seems to load twice, even from the graph. I understand that it would load once for no route, then when the route is set, but it seems to do that twice? Something I'm keeping my eye on and will show on our next call. 👀

[hutchgrant]

No longer using lit-apollo

edit: but understandable if you wish to highlight the project because it inspired the apollo client wrapper.

[thescientist13]

Hmm, yeah. I ended up taking it out. If anything, the idea of the client wrapper is probably more inspired by the Proxy Pattern from the Gang of Four's design patterns.

[hutchgrant]

oppourtunirty

typo

[thescientist13]

phew... where's my spell checker plugin at? 🙈 🤣

[hutchgrant]

This is that the schema looks like:

This is what the schema looks like

[hutchgrant]

Graph is a subheading of queries, but it shares the same font-size due to both being h4. What do you think about the organization here?

[thescientist13]

I suppose I can just add one extra # to each heading then. But maybe better to consider @data as it's own package, as since it will be such a key feature, it might make sense to make it its own navigation item, which would give these docs more opportunity to breath, as it were.