Meteor 1.3.2.1 - Breaking Changes to Fast Render
Closed this issue · 15 comments
The changes made to livedata in Meteor PR meteor/meteor#5680 (which were shipped with Meteor 1.3.2.1) cause fast-render to stop working (and actually cause general funniness to apps using it). The built-in package tests are failing, and any app utilizing it will fail too.
I have a barebones sample repo here: https://github.com/abernix/broken-subs-1.3.2.x but again, the fast-render package-tests probably cover it.
Confusingly, the error message states:
Error: You can't use reactive data sources like Session inside the `.subscriptions` method!
However, that does not seem to be the actual reason that it's failing, and that error comes from FlowRouter. I haven't had a chance to tear it apart and figure out a fix for it, but I thought I'd at least open a record of it.
Okay. I'll look at this.
Ok. The change comparison here for livedata shows what's up (among a couple other small, unrelated things): meteor/meteor@release-1.3.2...release-1.3.2.1
BTW: This seems to be coming from FlowRouter. Could you try the flow-router-ssr version and see this is there too?
Unfortunately, I'm using Blaze so I couldn't try flow-router-ssr.
Anyhow, yes, forgot to say that the error itself is from FlowRouter, but if I simply remove fast-render, everything works fine (with no other changes at all).
I concluded that since the changes were to livedata and that fast-render seemed to be what was working with livedata (by wrapping the _livedata_data method, which was changed in Meteor) that the error likely lay in FRender somewhere.
Confirmed, routes are broken using Meteor 1.3.2.2 with FastRender + FlowRouter. Also using Blaze.
@arunoda It appears that Meteor released this livedata change mistakenly in 1.3.2.1 (never marked recommended release) and 1.3.2.2 (which was recommended release for about 24 hours, but is no longer). They will be rolling it back in 1.3.2.3 and shelving it until 1.3.3 (which is where it was supposed to go).
As to whether this works in flow-router-ssr: I found your https://github.com/kadira-samples/meteor-data-and-react sample (Meteor 1.3 + flow-router-ssr + ReactMounter) and it worked fine when upgraded to 1.3.2.1. So it appears this problem will affect Meteor 1.3, fast-render, Blaze users.
Right, I'm just saying that it appears flow-router-ssr works fine while flow-router + fast-render does not. Unfortunately, because of Blaze, flow-router-ssr is not a solution in my situation.
The problem goes away if you get rid of any subscriptions that are specified in in your route definition. It's possible that FlowRouter is just incorrectly detecting reactive components now & throwing an error, thus breaking things.
But still, fast-render unit tests start failing under Meteor 1.3.2.1.
Hi, not sure if this will bring some new info in addition but Roger marks some issues on local collection observer as well: see forum entry
@mitar and @nathan-muir can probably provide some insight that may help solving this one. I suspect the issue is that with DDP batching, callbacks from various DDP messages may not run in the same tick that the message arrived.
Hmm, I'm not a fast-render render user, so I've never looked into the details of how it works.
It looks like fast-render converts added's to changed if the document exists somewhere - It might just need to check self._bufferedWrites on these lines
I had some comments on how this is implemented here. I think we could move to having complete client session established from the server. I didn't have time to do it in the past. Maybe it is time. ;-)
@nathan-muir and @mitar, thanks for your input, and also your buffering addition to livedata – super awesome 🎆 and much appreciated. Hopefully the PR I just submitted will fix this, but I'd appreciate it if you could review my reasoning on it.
It appears to be two relatively harmless things throwing false positive errors.
- The package tests were failing because of a timing issue (since they're expecting the collection changes in the same tick as the
livedatainjection) and were fixed by changing them to be async with a delay ofbufferedWritesInterval(5). - For a similar tick reason (and as @tmeasday had thought),
flow-routerTracker invalidation was throwing an exception.
... more details in pull request #167.
Discussion wise...
Contrary to what @nathan-muir had suggested, checking _bufferedWrites didn't seem to have an effect pro or con. However, should I change this to merge anything that is buffered in the same way as the _updatesForUnknownStores?
And @mitar, I didn't quite see where that stuff came into play, but honestly, this was my first trip down livedata lane. 😁 Perhaps we should discuss further?
And lastly, @arunoda of course, whatever you think - it's your package. There might be some double (unnecessary but harmless) flattening of collection data going on with the new DDP buffering.
I didn't quite see where that stuff came into play, but honestly, this was my first trip down livedata lane. 😁 Perhaps we should discuss further?
So my is just hypothetical, I am not completely sure about it either. But the whole point would be that we would use DDP client code on the server to establish a real connection to the server, but from inside server. Then all the code for merging fields, keep state and so on would work without any changes, hopefully.