Skip to content
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

Wrong merging of documents #161

Open
mitar opened this issue Feb 28, 2016 · 3 comments
Open

Wrong merging of documents #161

mitar opened this issue Feb 28, 2016 · 3 comments

Comments

@mitar
Copy link
Contributor

mitar commented Feb 28, 2016

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
Copy link
Contributor

arunoda commented Feb 28, 2016

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
Copy link
Contributor Author

mitar commented Feb 28, 2016

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
Copy link
Contributor Author

mitar commented Feb 28, 2016

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:

  • you crate Context
  • you run all rendering of a component while Context is active
  • we wait for all subscriptions to stop, or we timeout
  • you ask Context for all data

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants