kadirahq / fast-render

Render you app even before the DDP connection is live. - magic?
MIT License
560 stars 80 forks source link

Use real subscription context #160

Closed mitar closed 8 years ago

mitar commented 8 years ago

Pull request for #159.

mitar commented 8 years ago

Because I use official code to publish, not accounts-base returning null instead of calling ready() is showing a warning. I opened a pull request to fix that: https://github.com/meteor/meteor/pull/6363

mitar commented 8 years ago

So this seems to work for me. I made it so that the publish context is really the official one. Also much of the official code is reused. I also fixed some small bugs which I discovered (for example, passing values as reference instead of copying it, and not removing fields in changed callback when value is undefined).

I would guess we could go even further and even make our "session" object the official one, and make it so that it keeps internal state of documents and all merging across subscriptions could be reused, we only make it so that when session sends a message, it does not really send a message to the client, but to our copy of client data on the server. So we override the session's sendAdded and similar methods. I think this would make everything much easier and completely compatible with Meteor (with all diffing and merging and stuff done correctly).

arunoda commented 8 years ago

I think this is great. Merge box related stuff goes to a different PR I guess after this. There's one more we need to do.

There are many apps (and yes accounts package) returns null thinking it'll stop the publication. We need to handle it with by overriding _publishHandlerResult.

On there, we need to convert null into [].

mitar commented 8 years ago

There are many apps (and yes accounts package) returns null thinking it'll stop the publication.

But how does Meteor handle this? How does it know that client-side does not have to wait for .ready() and it can render things?

mitar commented 8 years ago

BTW, publishHandlerResult is a separate function only in development branch. This was a pull request I made. So now it is easy to fix this, but it will not work for older versions of Meteor where publishHandlerResult is part of the runHandler.

arunoda commented 8 years ago

@mitar Okay got it. (yeah, the code looks fresh to me). Then we may need to override handlers, but that's hacky.

In the accounts case, it's a universal publication and it has no meaning of ready. Some users do the similar in universal pubs and some don't do ready checks.

This is a pretty big issue if done in the FR related code. (Since this leads to 500ms delay in the server).

mitar commented 8 years ago

OK, I will address this by wrapping the handler itself.

arunoda commented 8 years ago

And then, you may need to wrap, Meteor.publish for runtime and publication loaded after Fast-Render.

mitar commented 8 years ago

I looked into this a bit more. In fact, universal subscriptions do not even have ready and stopped state. So this is in fact a completely arbitrary choice for us when we decide it is "ready".

I would propose that instead of checking for null, we just assume all universal subscriptions are ready immediately after their handler exits.

arunoda commented 8 years ago

@mitar Okay. That's a good idea. For other cases, we'll add some note here: https://github.com/kadirahq/fast-render/blob/master/lib/server/context.js#L102

mitar commented 8 years ago

Fixed.

arunoda commented 8 years ago

Awesome. This looks great to me. I'll take this in.

arunoda commented 8 years ago

Merged and released as v2.13.0