-
Notifications
You must be signed in to change notification settings - Fork 23
Conversation
@@ -69,4 +75,7 @@ export { | |||
setRequiredExternalsRegistry, | |||
clearModulesUsingExternals, | |||
getModulesUsingExternals, | |||
getStoredPromise, | |||
storePromise, | |||
useStoredPromise, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expose only the helper functions, to maintain abstraction for future underlying changes
8ce0c46
to
a4b15c2
Compare
dispatch = thunk.withExtraArgument({ | ||
...extraThunkArguments, | ||
rebuildReducer, | ||
modules: store.modules, | ||
promiseStore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attach the PromiseStore to the extraThunkArgs
a4b15c2
to
58e3fc3
Compare
*/ | ||
class PromiseStore { | ||
constructor() { | ||
this.promises = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make is private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and maybe a Map
? if not a null-prototype object (Object.create(null)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed both in cbe1f89
* @param {Promise<any>} promise The promise to be stored | ||
* @returns {void} | ||
*/ | ||
store = (domain, key, promise) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
store = (domain, key, promise) => { | |
set = (domain, key, promise) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, I guess I then didn't articulate this, in code or description.
I was thinking about having the store function throw, or silently no-op if the promise already existed. There is no reason to even overwrite a promise, so a collision would indicate something has gone wrong.
Because of this, I didn't like set
because i would expect set
to both store
and update
depending if the promise already existed.
* In general, Module code should not interact with the promise store directly, | ||
* instead dispatching the below thunks, or using the provided hooks | ||
*/ | ||
class PromiseStore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cant see a reason to delete a promise.
The purpose is to share promises across modules a LoadModuleData time.
At what point could you delete one of these promises? a single loadModuleData function does not know where it is executing, it does not know if the promise its just about to delete is still needed by some module that is going to be 'composed' in after the current moment.
So i cant think of a usecase where deleting a promise would be desirable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can they expire? Will this grow infinitely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are scoped to the request. So in the server when the request finishes it will be garbage collected.
In the browser, it will only grow as much loadModuleData
is called, and only grow as fast as the fetchye cache grows.
Can we properly serialize everything about a Promise to transport it from the server to the client? |
I dont think it perfectly replicates everything about the promise for Streaming. Just the basics. |
💡 Feature request: Holocron
PromiseStore
Holocron comes with a sophistocated
DataStore
, TheDataStore
is pluggable, shared across the entire system, and scoped to each request in the server.At a high level, the
DataStore
is used by:holocron
itself as aModuleStatusStore
modules
as aModuleStore
fetchye
andiguazu
as aCacheStore
(although technically these stores are registered by the root module)ConfigStore
, anIntlStore
, aRedirectionStore
, and aRenderingStore
. This looks a little like this: (organised by domain and type, not structure)This system has allowed Holocron, One App, fetchye etc. To solve many problems when it comes to Two Phase SSR, Client Hydration, Api De-duplication... The list goes on.
However, there is an underlying technology that prevents this versatile store being used for Promise's: transmitJS
The DataStore is serialised and sent to the client, where it is deserialised. Promises cannot be meaningfully serialised, so cannot be put in the
DataStore
.So why do we need a Promise Store?
A
PromiseStore
would serve the same purpose as theDataStore
, but for promises: To share promises across the system, scoped to the individual request, and even send those promises to the client.In the immediacy, the promise store would serve as a solution to this bug in fetchye: americanexpress/fetchye#93, which requires Promises be shared across the module boundary.
Additionally, The up coming React Streaming feature in One App requires not only to share promises between modules, but then to be able to send these promises to the client, and hydrate them in such a way they can then be resolved at a later date. ( I mentioned above that Promises cannot be serialised, and this is true, but React Streaming contains a way of passing data from the server to the client, the resolving the client promises with that data, which makes it look like the promise was resolved across the server/client boundary, this would be integrated with the PromiseStore if streaming is enabled ).
Since it is Holocron's responsibility to configure, maintain, serialize, and hydrate the
DataStore
. I beleive it should also be Holocrons reponsibility to do the same with a hypotheticalPromiseStore
So how would this work.
In short, a new object will be attached to
extraThunkArguments
, that serves as a promise store.New Thunks and Hooks will be added for accessing the promised from this object, in an abstract enough way that we can change the underlying fabric of the PromiseStore freely in the future.
This PR serves as a reference implementation of whats needed, Now is a good time to jump to the diff, have a read of the code, then come back up here and read my predicted Questions and Answers.
Note: This is not a final implementation. It explicitly lacks unit tests to keep the diff focussed, and may need tweaks when integrating this change into One App and fetchye.
Some questions you might have while reading the PR:
How would Fetchye use this?
The
makeServerFetchye
function would be extended to be optionally passed a PromiseStore (any object with astorePromise
andgetPromise
function on it, non-one-app users can bring their own, as they do the cache).in
makeOneServerFetchye
, it would pass thePromiseStore
from holocron intomakeServerFetchye
.If there is a
PromiseStore
passed intomakeServerFetchye
it will first check for the existence of a promise in the store for the computed cache key. If it exists, it wouldawait
it, then return thecoerced
fields like normal.This would mean that the second call to the same data would 'inherit' the promise from the first, and act correctly.
Why attach the
PromiseStore
to theextraThunkArgs
?extraThunkArgs
is Holocron's way of sharing "things" system wide, such as thefetchClient
and therebuildReducer
function. We rarely expect our consumers (Module Code Authors) to interact with theseextraThunkArgs
, instead it serves as an internal transport for "things" that we need anywhere.Why no Hook for storing a promise
This
PromiseStore
is currently focussed on solving two problems. One Now (the fetchye bug) and one in the future (React Streaming)Neither of these require the ability to store a promise at request time. But always want to store promises at 'LoadModuleData' time.
Any more questions?
Just ask in a comment :)
Alternatives Considered
Both the fetchye bug, and the Streaming system have considered alternatives to this solution, and infact streaming is working partially without this feature. However both cannot overcome all issues they face without some iteration of a 'System Wide'
PromiseStore
Don't attach to
extraThunkArgs
I considered this at length, however ultimately to satisfy both
System Wide
andScoped to a Single Request
, we would simply need to re-engineer thedispatch
system and attach thePromiseStore
to that. Which adds needless complexity.Just actually store the Promises in the Redux Store, and handle the serialisation separately.
This was considered (and actually implemented for streaming), however its quite messy, and leaves us with far more code to maintain than the above does.
Futhermore, the Holocron
DataStore
isimmutable
and Promises are not an immutable data type, therefore Promises don't belong in the DataStore for more than one reason.