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

Discussion of preventing mergebox plus non-reactive rendering #894

Closed
etyp opened this issue Sep 25, 2015 · 5 comments
Closed

Discussion of preventing mergebox plus non-reactive rendering #894

etyp opened this issue Sep 25, 2015 · 5 comments

Comments

@etyp
Copy link

etyp commented Sep 25, 2015

At the September Devshop talk, there was a section on preventing mergebox, Meteor streams and performant rendering that I feel is quite important for projects of all types. I was hoping to get a discussion about how the mergebox was prevented, the use of non-reactive templates with konecty:nrr, and off dom rendering.

The talk gave a high level overview and looking at the source is very helpful, but discussion and explanation could be very valuable and make a great wiki (thanks to @gmsecrieru for the suggestion).

Can anybody take a stab at explaining any of these components in the scope of Rocket.chat?

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@rodrigok
Copy link
Member

Hi @typ90

Mergebox: you can reduce the mergebox reducing the use of subscriptions, you can use some package of streams to send data to client in certain cases

NNR: is basically render some template as HTML with the desired data to a JavaScript variable, this disable all reactiveness in the rendered template and reduce the amount of computations

Detach DOM: Just remove the part of the DOM that is not visible to user and keep in memory (javascript variable)

@mitar
Copy link
Contributor

mitar commented Nov 11, 2015

OK, I checked the video and I have some questions about that. So I was looking into how to optimize Blaze before but I had a different take. Maybe just because none of my projects were hit with hackernews. :-)

I think a better way would be to really disable merge box instead of using streams. I opened a ticket for this: meteor/meteor#5645 From my past looking into the code that should be probably quite easy to do. I would even say that probably a 3rd party package could do it. So then you could just mark publish as having no merge box and all documents would be automatically send, and all other code could look exactly the same as Meteor code. My hacky approach would be to simply clear all fields in getCollectionView(collectionName).documents (you would still keep information which documents are published, but not which fields) after every added, changed and removed. I am touching those internals already here. Now that I think, this could be a very simple and quick package probably.

About Blaze, what is the problem with computations. Is a problem that there are too many computations, or is it a problem that too many of them are invalidated unnecessarily? My experience is that it is not a problem if you have many computations, the only issue is that if many are invalidated and then they have to run, just that they discover that no DOM has to be changed, or that a small piece of DOM has to be changed. But in your case you are claiming that invalidations (data context changes) occur rarely anyway?

So from my experience is that by default Blaze invalidates a lot, just because its data context equality is defined very conservatively: any document is always different to any other document. Only primitive values are really compared. I have seen in nrr code that you even expect cursors as data contexts? This really makes it invalidate reactivity all the time.

So my solution to the problem is that I use computed field. So I wrap all calculations and accesses into one more autorun (so more computations) but which trigger invalidation of outside autorun only when resulting state really changes. In this way you make changes do not propagate.

If you want a comparison. Your current approach is more React style. You detect state change and you pass new state from top to a whole fragment to rerender, and then you replace that fragment (React would diff it and only update changes, but it is good enough what you do). Blaze allows a different approach, which is that also helpers internal to the fragment can access outside state (like Minimongo). And that if that changes, it will signal upwards that things invalidated and get things to rerun.

Now, in your case you are allowing reactivity to rerender only from above, so when a data context changes, but you ignore any reactivity inside the fragment. This makes it hard to reuse existing Meteor code inside the fragment, which expect it be rerendered and any reactive changes have to be put into the message data context.

Another issue is that by default in Blaze template helpers are bound to data context, so they are all invalidated every time any of data context change, even if template helper does not even use or use just data context partially. Blaze Components and computed fields addressed that and from my experience (but not really any dedicated benchmarks) things are really much smoother now without any real change in expressiveness of templates.

So, my questions are:

  • why you rather not use something like computed field which limits the unnecessary propagation for deeper levels upwards, which then in most cases mean that things are rendered just from the outside change (like in your case nrr), so when outside data context data change, but, you still support internal reactivity; so you can limit what exactly means that something changed
  • why you do not still allow internal (inside the fragment) reactivity to still invalidate things, but just then you rerender the whole fragment once that happens

So what I am thinking is that with the first two things you are really going against Meteor so it is a question for me how much is then left of Meteor. :-) So this is why I am thinking of ways to make it so that it is more Meteor-like, so that also others can easily adopt similar approach, without having to sacrifice things or adapt new patterns (streams, non-reactivity).

BTW, this is really ugly. I think it could be cleaned up a lot. (Edit: See #1376.)

cc @wh0

@mitar
Copy link
Contributor

mitar commented Nov 11, 2015

For example, see this comment from @blaskovicz. This is exactly what happens. That people want to get outside state inside template helpers and elsewhere, as they are used in Meteor, but there is no reactivity anymore.

So all state should be passed only through a data context (BTW, you could use RocketChat.callbacks here to populate data context), or inside content should still allow to trigger invalidation, only that the whole fragment would be rerendered.

(Or you can just use computed field to minimize rerendering.)

@mitar
Copy link
Contributor

mitar commented May 4, 2016

See #1710 for my proposal to use control-mergebox package to fix this.

@MartinSchoeler
Copy link
Contributor

I am going to close this issue due to the lack of any update for so long and the rapid speed of changes that has happened with Rocket.Chat. If you happen to experience this with the latest version, please feel free to open it again with updated details. Thanks

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

No branches or pull requests

5 participants