kadirahq / fast-render

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

Wrong merging of documents #161

Open mitar opened 8 years ago

mitar commented 8 years ago

While working on #160, I noticed that the package is potentially processing equal documents in invalid way and potentially send multiple same (same _id) documents to the client unnecessary instead of merging it on the server.

Here you are pushing collData which is an array of documents onto a collection. In this way you get array of arrays. Later on you add arrays. And you push this array of arrays to the client, at least it seems so. There is no code which would be merging multiple documents with same _id together. At least I do not see it. You just combine arrays. And have arrays of arrays even.

The second issue is that in the flow-router SSR you are deep-merging documents. This is wrong. Meteor does only top-level merging. This might introduce unnecessary incompatibilities.

The third issue is that you should probably validate inputs into the update call and check if there are any top-level keys which start with $. Because added/changed/removed calls do not check that you might get a document with those in, from bad code or malicious code.

arunoda commented 8 years ago

Fast Render Issue

Here's the client-side merge box like functionality I used - https://github.com/kadirahq/fast-render/blob/master/lib/client/fast_render.js#L41 (Anyway, now I think it's better to do that in the Meteor's way if we can use it)

FlowRouter SSR issue

I think yes. That's right. We need to do shallow copy I really like to have some help on this.

mitar commented 8 years ago

Here's the client-side merge box like functionality I used

Yes, but why client-side? This means that the injected data can be unnecessary huge! You could have same documents multiple times send to the client.

(Anyway, now I think it's better to do that in the Meteor's way if we can use it)

Yes, I think we could do that. We would just create a our own session object and then reuse the rest of Meteor codebase.

We need to do shallow copy

You could just use _.extend. :-)

mitar commented 8 years ago

I really like to have some help on this.

I could look into using sessions, but then the API will change to one returning documents themselves for the whole session (which would be one per one HTTP request made). But integrating this new API with the rest of your packages is probably best done by you.

So if I understand correctly, you currently create Context for one HTTP request, yes? I could do it so that internally it has a session object and then all subscribes work against that session object. And then you have a method to inspect data inside that session object.

I would remove that subscribe returns data. This looks unnecessary complication. So API would be somehow like:

I do not know when I will have time for this though.