-
Notifications
You must be signed in to change notification settings - Fork 159
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
adds fetchAsync to useFindServer #360
Conversation
if (cursor?.fetchAsync) { | ||
return cursor.fetchAsync() | ||
} |
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.
could you update the warning here to add the fetchAsync
in here as well?
+ 'Make sure you do NOT call .fetch() on your cursor.' |
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.
Great job @eduwr 💪🏻
I'm not an expert on the Meteor-React integration, but I'm certain it won't work as soon as we make |
@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):
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 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 |
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 |
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.
Or did I miss something? I guess, for now, the |
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 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. 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.
This would make maintaining the current |
Possibilities to fix
Introduce a new synchronous fetching function. It could be called 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.
The 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.
For now, the There are two ways to implement it: 4.1. First approach: Edit the 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 4.2. Second approach: Extend 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 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 Now, we can preserve this behavior on the client but not on the server. As I already wrote on the forum:
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, 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. |
It would but in a good way. Right now, when With the |
@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.)
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 |
Since the
But then React docs say:
(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 To see if it works, we could actually use an existing integration, like Relay, use @piotrpospiech, @CaptainN, @Grubba27 What do you think? |
I REALLY prefer the relay approach. Sounds easier to explain and easier to get started. |
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
I tried to use Streaming SSR works fine if I use
The onPageLoad((sink) => {
sink.renderIntoElementById("react-target", renderToString(<App />));
}); In this case, we can use onPageLoad(async (sink) => {
const App = (await import("/imports/ui/App")).default;
hydrateRoot(document.getElementById("react-target"), <App />);
});
In this case, I used 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 Summary
We can do a Suspense-ready |
@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 - 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:
...with the current 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., meteor/meteor-feature-requests#423). Without it, the client will render the fully-rendered HTML and immediately replace it with spinners, because the |
What would you suggest @radekmie? bring this changes to the internals? Or something different? --
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. |
|
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 Of course, another option is to 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:
|
I think that rendering anything without |
I agree with @radekmie that anything without 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)