Fix SSR issues #94 & #95 & (somewhat incidentally) bug #91

[zenflow]

I started working on fixing #94 with my proposed solution (force fetchPolicy to 'cache-first' when on server or first render on client, and render+wait repeatedly in getDataFromTree) but found that getDataFromTree was still entering an infinite loop, since store.query() still pushes a promise (to store.__promises) when it's a cache hit (i.e. just getting the results from the cache, synchronously).

So it was necessary to rework the control-flow of the Query class, so that we only store.pushPromise() when there is some actual async work to do, and otherwise just set the results (data or error) and the promise (which is already settled). The current code in master follows a shared path of execution once it has data, regardless of whether it is from the cache or from a fresh response (and with no use of a param/flag to indicate which). That is also the cause #91. I separated out two execution paths fetchResults & useCachedResults, and simplified the async control-flow (I have never found it helpful to pass around a promise's resolve & reject handlers. There is a reason they are only meant to be called inside the promises callback, as in new Promise(callback)). Instead of repeating the problematic code in the new useCachedResults path, there I put the data through a new store.deflate method (removes data properties from objects of known types) before putting it through store.merge (only if query is not "raw"). This is all done in 00a8366.

I didn't make the 2nd step (see #91) of removing the (above mentioned) data properties of known types from the __queryCache (which would be awesome) because the query method can be called with the raw option, and that will need the full, inflated (un-deflated) data to be in the cache. To deal with that I propose a new option for the RootStore constructor: cacheMode: "raw" | "normalized" to indicate how you want your data cached. With 'raw' things behave exactly as they do already. With 'normalized', the data is put through store.deflate() before saving to __queryCache. The data no longer needs to be deflated just before mergeing. If query is called with raw: true, the data should be put through a new store.inflate() method before being returned, which will add the data props back to objects of known types. This way we can save many bytes in __queryCache (for SSR or serializing/deserializing for localStorage) while keeping first-class support for "raw" queries.

@chrisdrackett @mattiasewers What do you think of my proposed cacheMode: "raw" | "normalized" option? We could default it to "raw" for now, since that is the current behavior, and in the version 1 release switch it to "normalized" since that is (I imagine) what people would want in most cases for performance.

The other commits in this PR are pretty straight-forward I think.

• 2a8950f Fix SSR, closes [SSR] getDataFromTree ignores (doesn't wait for) queries triggered after loading first queries #94, closes [SSR] Queries made on server are repeated on client when page loads #95
• c726b82 Add noSsr: boolean query option to skip fetching query on server side
• bf4e35a [5-nextjs] Demo (1) layered SSR queries. (2) noSsr query option

[zenflow]

Btw, I believe this PR is ready for review and merging. This fixes the essential problem of #91 & two other issues and we can add the new cacheMode option in a follow-up PR.

[chrisdrackett]

@zenflow this looks good for me and appears to work in the examples, so that's good!

That being said, some things are a bit out of my wheelhouse. I'm wondering if someone else can review this as well before we merge. @elie222? @mattiasewers?

as for the cacheMode are their downsides (other than doing more testing) to just defaulting to normalized vs. adding an option? Is it that we need to add a inflate functionality first?

[zenflow]

as for the cacheMode are their downsides (other than doing more testing) to just defaulting to normalized vs. adding an option?

The downside of normalizing by default is making "raw" queries just a tiny bit not as nice:

• Extra work first normalizing & converting it to MST objects, then de-normalizing & converting to plain js objects. Could be noticeable for large sets of data.
• Possibly receiving extra data props that weren't requested. Good & fine when we are working with our model instances, but seems unexpected & maybe undesired when working with raw requests.

I don't actually have much of a use for "raw" queries or a "raw" cache mode right now, but I think we should fully preserve it since I could see it being useful in some situations, maybe debugging, to completely circumvent all the crazy things we're doing in our models.

[zenflow]

I don't actually have much of a use for "raw" queries or a "raw" cache mode right now, but I think we should fully preserve it since I could see it being useful in some situations, maybe debugging, to completely circumvent all the crazy things we're doing in our models.

Actually, I can't really imagine any uses for "fully raw" queries at all.. even for debugging because I can see the network response in chrome devtools.. and obviously even that would not have been enough to make the optimization.

When someone passes raw: true for a query, they just want to get back js objects instead of model instances, for whatever reason. It's really pretty simple. The data can come from the same place as with non-raw queries.

[zenflow]

@chrisdrackett I must agree now that there's no need for adding an option, and we can just always keep a normalized cache.

I can add the inflate functionality easily (following the pattern of merge & deflate), and make the changes to keep cache normalized.

I will do that soon and commit it in this PR if it hasn't already been reviewed & merged. See #97 (comment)

[zenflow]

Made one major fixup: bc7da9b Fixup 00a836, in deflate(), only remove props of root type objects. If we removed props of objects of types that are known but not root (as my code was doing before), then we would never get them back (in most cases, i.e. unless you've saved the instance in the store yourself) because merge() only adds instances of root types to the store.

[zenflow]

00a836 is a typo. It should be 00a8366.

[mtsewrs]

@zenflow Good job! I've gone through everything and nothing seems to break tests, types or examples.

@chrisdrackett This can be merged whenever you see fit

[zenflow]

@chrisdrackett do you think this could be merged? (I don't care when it's released) I prefer to do the normalizing __queryCache bit in a follow-up PR if that's ok. Also I think it still needs a bit of discussion first, see issue #99 which I just opened to carry on the discussion.

[chrisdrackett]

Yeah, lets get it merged and I'll work on getting a release cut later this week!