search

kadirahq / fast-render

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

Allow change/remove of other pubs docs to fix #142 #154

Closed brettle closed 8 years ago

brettle commented 8 years ago

Change PublishContext.changed() and PublishContext.removed() to ensure that the collection exists, and if there isn't a doc with the requested id in the current publish context, to look for one that was published in another publish context. If one is found, changed() adds the doc to the current publish context and removed() does nothing. If there isn't, both throw the same errors that would be thrown by the merge box.

arunoda commented 8 years ago

Hey, I got the issue. Currently we re doing it with running a merge-box in the client. See: https://github.com/kadirahq/fast-render/blob/master/lib/client/fast_render.js#L39

May be this is related to that. Could you send me a sample repo with the error or try to improve it on the client side. I don't like to build a kind of merge box on the server for FastRender.

brettle commented 8 years ago

Thanks for the quick response. I was already aware of the client-side merge box you mentioned. Unfortunately, if you want to get behavior that is equivalent to the official Meteor implementation, a client-side change won't work. The issue is that the official changed() and removed() throw an error (on the server) if the object isn't already in the collection, and don't throw an error if it is. By the time the client-side merge-box runs it's obviously too late to throw the error on the server.

That said, I don't think we need anything approaching a server-side merge-box implementation. If you are concerned about the potential performance impact of my PR (since a change/remove can result in looping over all docs in all pubs for a collection), I could change it so that it uses a more efficient data structure to keep track of which pubs contain a particularly id. Might that address your concerns?

As for a repo demoing the error, I'm not sure if you saw that the PR contains a test case that produces the error if the fix isn't in place. Let me know if you'd still prefer a separate repo and I'll see what I can do.

brettle commented 8 years ago

I went ahead and changed the PR to be more efficient. Let me know if you have any questions.

arunoda commented 8 years ago

I think we now have much better solution now. See: https://github.com/kadirahq/fast-render/issues/159