Closed eduwr closed 1 year ago
I'm not an expert on the Meteor-React integration, but I'm certain it won't work as soon as we make fetchAsync
truly async (i.e., not wrap fetch
's result in a promise). That's because Tracker
is not compatible with promises - reactive sources have to be registered (.depend()
) synchronously.
@radekmie I see. So this is the reason that in my PoC even returning a promise, it is being resolved anyway.
@eduwr I haven't checked the app, but yes - these promises will be resolved immediately.
My seagull view (I won't have a chance to review the code for a while):
A straight forward port isn't going to work, because this drastically changes the data paradigm.
In useFind (and useTracker) right now, we are relying on the fact that data is available immediately. So the algorithm roughly works like this in first render (the one that gets concurrentized):
useFind
we use an observer, in useTracker
, reactivity is set up by re-running the query in Tracker.autorun to regenerate the computation).This let's that first render really cook, as we can feed data through the react tree, and get the benefit of that concurrent rendering. It's also real easy to set up, and is inline with users' expectations (at least thus far).
In the new paradigm, it's async, so we have to:
It's not a complete bummer that we can't get data immediately. This will work great with suspense for data! But if we do it the traditional way, we'll lose some of the easy to set up benefit of concurrent rendering. It's not the end of the world, you can still get that benefit using well thought out component composition (treat the component with useFindAsync
as the "container" and only render the list component when you have data).
Anyway, it's different, so it'll need to be handled differently. Great to see the ball rolling!
I just realized this was for server side - here are a few notes about that:
We probably need a more robust SSR story for Meteor in general. It is already challenging to render code on the server, and I've not been able to do it successfully in larger projects without memory leaks (in Meteor/React projects). I'm actually hoping this effort will improve things.
In order for SSR to work right, there needs to be a system that does roughly the following:
In order to achieve all that, we'll need a Provider server side to capture and process that data, and we'll require the user to use it to get SSR. Then client side, we need a provider to hydrate the data for that first render.
OR:
If we just want to prevent things from breaking, then we need to simply not even bother trying to load data server-side, and instead just return some kind of state that says essentially "never going to load" - this could just be loading state, that will never complete. This would be needed in cases when there is no Provider anyway, or if the user opts out of SSR (another feature we should enable, on a hook instance by hook instance basis).
In cases where the server didn't load and render data, on client side, it'll render "loading" appropriately, then load the data, then fix itself when the data loads - so that's a perfectly viable short term solution.
@CaptainN As I checked, throwing promises is not an official solution for Suspense, but there's no other option as of today (the latest comment I found is from March). However, it works, so maybe we shouldn't be bothered by it...?
When it comes to a better SSR in general, I do agree that it would be great. But I also see, how much the development it'll take. And honestly, making the server always render a loading state doesn't sound bad to me.
Another alternative is to make it slightly more (far) future-proof: call cursor.fetch()
if it's there, and return a loading state if it's not. Alternatively, accept a "fallback value", that would be returned if the cursor is not ready. (This can be handled in the userspace as well, but mentioning it in the docs would help.)
Thinking about this a bit more - is there a strong reason the client side data access has to be async in all cases? There are a lot of reasons the data might already be available on that first call (sub has already loaded elsewhere, or localstorage backing, etc.). It would seem we could keep synchronous behavior client side, and worry about SSR some other way (using the algorithm I described, which sort of simulates synchronous access server side anyway).
If it's going to be async, we'll just need to use useEffect
and set everything up there. It'll actually make the hooks cleaner/simpler, at least client side.
[...] is there a strong reason the client side data access has to be async in all cases?
Well, there's none. The only problem is, that the code will no longer stay isomorphic. This is still not solved, though.
A Friedly ping so that we can come back to this discussion on what we should do. So far, what I've understood is that we have some options(not taking SSR into account for now)
Or did I miss something?
I guess, for now, the useEffect
option seems fine compared to others.
With this change, should all code pass in a useEffect right?
A note about isomorphism and React - SSR doesn't have to be isomorphic for React. Radical, I know!
Basically, the algorithm I described above, where we partially render, then discard and rerender, until all data is loaded, then do a final pass - that is the right way to do SSR in React today (it may change in the future). That's not at all how client side rendering works in React.
So basically, I'm not sure isomorphism is a necessary constraint for React support.
The useEffect
option would simplify things implementation wise, but then we are always rendering data after mount, which means we don't get any of the benefits of concurrent rendering (which only happens before mount). That is a core reason we didn't just do that in the first place. There are a bunch of other reasons too, some of them opportunistic. Meteor data is available in a lot of cases, in the first render, and it'd be a shame not to use it. On the other hand, maybe there's not a real benefit to using that - and we should measure to find out.
There are also entirely different paradigms for data loading that the react team seems to prefer, that aren't compatible with Meteor's observer model for setting up data loading and subscriptions (and I mean both reactivity from the server, but also the lifecycle of the observer on your reactive Meteor data source).
If we want to do server side considerations right, we should really start with the SSR story first, and build from there. There are currently many missing pieces to getting that right.
Just some food for thought.
Here's the complication. useTracker
and useFind
do not actually load any data. That's key to understanding all this is. These hooks just sets up an observer (or "computation") - this is its own side-effect, in react terms. Setting up that observer might trigger some secondary side-effects, like loading data through a subscription. It's the subscription that loads data, not the find method. Those side effects from subscribe operate from a black box, one that has some known challenging quirks born from its own internal state manager, which is still tied to some assumptions about Blaze's lifecylce. That's actually problematic, and has made maintaining the current useTracker
and useSubscribe
kind of challenging.
If I had infinite time/budget, I'd do these items:
As for the task at hand - supporting findAsync - I probably wouldn't even bother changing the find method on collections to be async, but instead write a synchronous version that works just as the current one does, which would simply hydrate data from the subscription (which is where data is actually loaded) even on the server side.
It's just too convenient to have synchronous data - and the only real problem is server side async behavior. It's all fine client side. And since it's just a problem on the server, and there are all these other problems anyway, I'd sidestep the issue by filling in those missing pieces, then emulate synchronous access on the server.
There really isn't a way to do this without at least thinking about all that context.
Hey @CaptainN, I hope everything is good out there! having read this a few times in this thread and searched throughout the community for solutions, I have seen two packages, and I'm not sure if they would work on the issue that you have pointed out.
The first package relates to SSR and async(react-streaming). I think having things working using sync functions is really good. I would love your opinion on this. Is this could work/something similar, or do we need to address something before doing things with SSR?
The second one is more of a workaround that we can use to have async code behave like sync core. This might solve this one for now, but I'm not sure what the implications of doing this are.
Rewrite Subscription from the ground up in a React friendly way. This wouldn't replace the current subscription system completely, since Blaze, Vue, Svelte, etc. still use it, but it should replace it within most React projects.
This would make maintaining the current useTracker
and useSubscribe
easier?
What are your ideas on this?
Possibilities to fix fetch
, as it will be asynchronous, in the useFindClient
:
Introduce a new synchronous fetching function. It could be called fetchSync
, fetchLocal
, fetchCached
, etc. The exact name is not important, but it will add the possibility to continue using synchronous calls on the client side.
That function should also be introduced to the server side, but it should throw an error or return an empty array. It's not perfect, but it's some way to handle isomorphic code.
Suspense
integrationThe two of the above options have drawbacks caused by moving from sync to async way, and there are breaking changes that could be problematic to implement in larger projects.
The component will be rendered twice, the first time in a loading state and the second after initial data is fetched.
The main issue is that we will query asynchronously for the data that we have in the Minimongo, so even if we can reach them immediately, we will be forced to handle the loading state. That will cause poor developer experience.
fetch
function from the useFindClient
and update data only by cursor observingFor now, the useFindClient
hook only uses the fetch
function for the initial call. Later all changes are handled via cursor observation. It's possible to remove that initial fetch that couldn't be made synchronously anymore and handle all data fetching by cursor observing.
There are two ways to implement it:
4.1. First approach:
Edit the useFindClient
hook to remove the initial fetch. It should look like this:
const useFindClient = <T = any>(factory: () => (Mongo.Cursor<T> | undefined | null), deps: DependencyList = []) => {
const cursor = useMemo(() => {
const c = factory()
if (Meteor.isDevelopment) {
checkCursor(c)
}
return c
}, deps)
const [data, dispatch] = useReducer<Reducer<T[], useFindActions<T>>>(
useFindReducer,
[]
)
useEffect(() => {
const cursor = Tracker.nonreactive(() => factory())
if (Meteor.isDevelopment) {
checkCursor(cursor)
}
if (!(cursor instanceof Mongo.Cursor)) {
return
}
const observer = cursor.observe({
addedAt (document, atIndex, before) {
dispatch({ type: 'addedAt', document, atIndex })
},
changedAt (newDocument, oldDocument, atIndex) {
dispatch({ type: 'changedAt', document: newDocument, atIndex })
},
removedAt (oldDocument, atIndex) {
dispatch({ type: 'removedAt', atIndex })
},
movedTo (document, fromIndex, toIndex, before) {
dispatch({ type: 'movedTo', fromIndex, toIndex })
},
})
return () => {
observer.stop()
}
}, [cursor])
return data
}
As the initial fetch was removed, only cursor.observe
is responsible for updating data from the cursor. This approach and the default behavior of this hook cause component to render twice. The only difference is that in this modification, the first result will be an empty array instead of fetched data. It needs to be handled on the front end and can be treated as a loading state.
4.2. Second approach:
Extend useReducer
with an initializer function. The only change to the previous example is to swap useReducer
with this code below:
const [data, dispatch] = useReducer<Reducer<T[], useFindActions<T>>, null>(
useFindReducer,
null,
() => {
const data: T[] = [];
if (cursor instanceof Mongo.Cursor) {
const observer = cursor.observe({
addedAt(document, atIndex, before) {
data.splice(atIndex, 0, document);
},
});
observer.stop();
}
return data;
}
);
As the data is initialized in the useReducer
, the React component will be rendered only once. There will be no need to handle the loading state as this approach synchronously fetches data without a transitional step. It will improve performance and developer experience as there will be no need to handle the loading state for just one tick.
This approach has the most advantages as there will be no need to handle unnecessary loading status, won't cause any breaking changes, and will reduce component re-rendering.
Just like @piotrpospiech wrote, we have these couple of ideas. Of course, 2., and 3. are React-specific, but 1. and 4. can solve the problem for other view layers as well (i.e., Blaze, Svelte, Vue, etc.). Please note, that 4. is actually 1. but without any additional API! The only thing that we rely there on is the fact, that observe
returns only after the initial addedAt
calls are flushed.
Now, we can preserve this behavior on the client but not on the server. As I already wrote on the forum:
As for the
observe
andobserveChanges
methods, I don’t have the answer yet. From the top of my head, making them return not onlystop
but also something likeisInitial
flag or eveninitialAddsSent
promise seems feasible, although I’m not sure if an entirely good approach.
After a while, I think an API like this could be enough:
const observer = cursor.observe({ ... });
observer.stop(); // Stops the observer. If any pending data is being loaded, ignore it.
observer.started; // `true` if all of the initial `added`/`addedAt` calls happened.
observer.startedPromise; // A promise that resolves as soon as `started` is true.
Of course, the names are not important now; the idea is. On the client, started
would be true
always (i.e., as soon as observe
returns) and the startedPromise
would be already resolved. On the server however, the observe
would return immediately and set started
to true
later, when the initial fetch happens.
Note that solves only the client part, not the server. We have to look into React's SSR to think about it, as we have to handle a promise there anyway (at least as long as there won't be any side cache with prefilled data).
Really liked 4.2 idea. If I understood correctly nothing would change for react users?
It would but in a good way. Right now, when useField
initializes, it uses fetch
to get the initial state, which results in a ready-to-use array on the first render. But as soon as it renders, the dispatch
call in useEffect
causes a second render with the same data (but not referentially equal).
With the observe
approach, there's only one render.
@Grubba27 In broad terms, I really think meteor needs a more complete SSR story (set of packages) for react. This should include solving async access to Mongo (with or without Fibers), and running the server side rerender cycle to collect all the data, collecting data for serialization (to be hydrated client side), and all that. Currently, it's possible to hobble together a solution - BUT, that leaks memory, so it's not ideal. It needs a real solution (like the one I described in a comment above.)
Rewrite Subscription from the ground up in a React friendly way. This wouldn't replace the current subscription system completely, since Blaze, Vue, Svelte, etc. still use it, but it should replace it within most React projects.
This would make maintaining the current
useTracker
anduseSubscribe
easier? What are your ideas on this?
Yes! A lot of the bug reports we get are from strange interactions between the React lifecycle and the internal "magic" that the Meteor subscribe function contains within it (which is pretty opaque, and somewhat tied to implementation details from the old tight integration with Blaze). An entirely purpose built data loader for react could alleviate all of that. I'd actually recommend doing that work in an implementation for useSubscribe
and deprecate the use of Meteor.subscribe
within useTracker
- we can kick up warnings without breaking anything, when that usage is detected.
Since the useFind
got sorted out on the client, let's talk again about the server side (actually, that's what this PR is about). I read the entire discussion again, checked how other frameworks do that, and honestly, I don't like any of the approaches.
Suspense
as it is (docs, beta docs) and calls it "streaming server side rendering". However, it's smarter on the client, as the initial bundle has all of the loaders there, and the rest of the content is just streamed to the browser. (The alternative is static rendering, where every component defines a separate function loading its data. I think it's an unrelated idea.)But then React docs say:
During server side rendering Suspense Boundaries allow you to flush your application in smaller chunks by suspending. When a component suspends we schedule a low priority task to render the closest Suspense boundary’s fallback. If the component unsuspends before we flush the fallback then we send down the actual content and throw away the fallback.
(emphasis mine)
So... Is that "good enough"? I couldn't find information about the time "before we flush", but maybe it's as easy as making useFind
use suspense for data fetching? I know, Suspense-enabled data fetching without the use of an opinionated framework is not yet supported., but Meteor is probably an "opinionated enough" framework. To check that, we'd need to throw
promises, at least until there's an official API.
To see if it works, we could actually use an existing integration, like Relay, use renderToPipeableStream
, and see if it works. If it does, then prepare a Suspense-ready useFindServer
and call it a day. This way the client-side will work as it does now, and to make the SSR work, you'll need to make it work with Suspense, i.e., render the Suspense
component above in the tree.
@piotrpospiech, @CaptainN, @Grubba27 What do you think? @piotrpospiech Do you think you could experiment with that and see, if that works?
I REALLY prefer the relay approach. Sounds easier to explain and easier to get started. Having a POC to see how it would work would be awesome.
I tested SSR with Suspense in the Meteor project. I used the meteor/server-render package. For testing purposes, I created a component that used the Suspense compatible fetch function, and it was wrapped in the Suspense
component.
I tried to use renderToNodeStream
as it is in the example, but it didn't work as expected. Website rendered only final response without loading state. I tried to enable streaming SSR in different ways (using renderToPipeableStream
), but it seems that the registerBoilerplateDataCallback can't handle streamed content.
Streaming SSR works fine if I use WebApp.connectHandlers
and renderToPipeableStream
to pass a stream to the response, as it is shown in this example, so it is definitely an issue with the meteor/server-render
approach.
The renderToString
function can be used to send from server to client an initial render, so in our case, it will be a fallback from the Suspense
component. However, if the fetch function is fast enough, it will send its final render.
onPageLoad((sink) => {
sink.renderIntoElementById("react-target", renderToString(<App />));
});
In this case, we can use hydrateRoot
on the client side to render the rest of the content (the component inside Suspense
instead of the fallback).
onPageLoad(async (sink) => {
const App = (await import("/imports/ui/App")).default;
hydrateRoot(document.getElementById("react-target"), <App />);
});
In this case, I used renderToPipeableStream
as renderToNodeStream
is deprecated. I used the PassThrough
stream because the stream returned by renderToPipeableStream
isn't readable. Perhaps there is a better way to do that.
onPageLoad(async (sink) => {
const pass = PassThrough();
const html = await new Promise((res) => {
pass.on("data", (chunk) => {
res(chunk);
});
const stream = renderToPipeableStream(<App />, {
onAllReady() {
stream.pipe(pass);
},
/* other options */
});
});
sink.renderIntoElementById("react-target", html);
});
This will send the entire page content when it is ready to the client, even if multiple nested Suspense
were used.
Summary
We can do a Suspense-ready useFindServer
, which will work with basic SSR.
@piotrpospiech I think this looks good, one note though - if you remove the initial getter (it's not a fetch - the data may already be on the client, and Collection.find is a synchronous operation - subscribe
sets up a fetch, not find
, and the observer
is a side effect) from the first pass render (as described in 4.1 above) you nerf the benefits of concurrent rendering by default. Rendering the complex data would only happen later, after mount, which doesn't render in concurrent mode.
Also, be careful about using useReducer's initializer, because it will NOT respond to deps changes, which can lead to edge case problems, like when React Router reuses mounted components between routes- this kind of thing has created problems in the past.
Just to add one thing to what @piotrpospiech said:
Streaming SSR doesn't work.
...with the current server-render
package. It doesn't mean we could make it work, though.
Thanks for chiming in @CaptainN! The changes on the client side already happened in #366, #370, and #374. It may be the case that some edge cases appeared, but none of the tests have detected not, nor anyone reported it (so far).
When it comes to a "full blown SSR", there's one more thing that needs to be done, and it's not React-specific: getting the Minimongo's data together with the initial HTML (e.g., https://github.com/meteor/meteor-feature-requests/issues/423). Without it, the client will render the fully-rendered HTML and immediately replace it with spinners, because the useTracker
on the client won't have this data just yet. It's a general topic and should be tracked elsewhere.
the Minimongo's data together with the initial HTML
What would you suggest @radekmie? bring this changes to the internals? Or something different?
I dig the idea of having an init
function and I guess the community is used to it already
--
Streaming SSR doesn't work.
We could try patching the server-render as @radekmie said as well, we already agreed that we could make something react-oriented at first and then try to generalize later. If this can be helpful at all there is this lib that I guess can help us obtain our goal.
@Grubba27
server-render
package. It has to be somewhere, though.server-render
"forward" streams and not only "aggregate" them. Or even WebAppInternals.registerBoilerplateDataCallback
, I don't know yet.When fetching data, can we also consider pages without a subscription? One significant use of SSR (aside from usability) is SEO. And SEO pages are for guest users; these pages typically only use methods (and not subscriptions) to fetch the data.
This will require using Suspense with the concurrent mode of React, i.e., stream the Html skeleton first to the client and then stream the suspended components once the data (from a Promise) is resolved per component.
Therefore, this will require renderToPipeableStream
. Aside from supporting stream as part of data (maybe data.stream
), I am unsure how to incorporate the boilerplate template as part of the skeleton Html initially streamed to the client.
Of course, another option is to stream-to-string
the entire stream when completed, as mentioned above.
Maybe we can focus on that first (send the entire stream for Meteor 3.0) and then figure out later how to support Suspense + Concurrent mode SSR
Thinking about this as a middleware, something like this might work:
const stream = renderToPipeableStream(
<App />,
{
onAllReady() {
const html = streamToString(stream);
sink.renderIntoElementById("react-target", html);
next();
}
);
I think that rendering anything without useFind
in it should be just fine, but let @piotrpospiech comment on that.
I agree with @radekmie that anything without useFind
will work, except for streaming SSR. We can send from the server initial render or the entire stream to the client and it will render the final result.
However, Suspense + Concurrent mode SSR is nice to have if we want to support the "full-blown SSR".
Getting data from methods (no subscription) pre-Meteor 3.0 is straightforward because methods use fibers to return values synchronously. With Meteor 3.0, it must now also wait for the promise to be resolved which is a good candidate for SSR + Concurreny mode of React 18
This one is now solved with Suspense.
Considering that Meteor release 2.8 is bringing a new MongoDB Async API (PR 12028), this PR introduces
fetchAsync
method onuseFind
hook.As recommended we should be using async methods on the server whenever possible to avoid future deprecation when fibers were removed. (Discussion 11505)