katowulf / mockfirebase

Firebase mock library for writing unit tests (experimental)
157 stars 40 forks source link

MockFirebase 1.0 Roadmap #83

Closed bendrucker closed 3 years ago

bendrucker commented 9 years ago

This is going to progress slowly, but I want to lay out the path towards 1.0 and offering better support for new Firebase features, especially queries, as they arrive. I've broken things up into headers for easier reading.

ES6

Since this is going to be almost entirely a ground-up rewrite, I'm going to write everything in ES6 and transpile it via Babel. I've been doing this for 100% of my front end work for about 8 weeks now and it's been a nice productivity boost.

Immutable Data, Observable References

The biggest obstacle to new feature support right now is the way that data changes are propagated. At present, everything is explicitly passed around via internal methods. Queries are especially challenging to understand. I'm going to build a new data structure using immutable. Instead of trying to figure out what reference paths need to know about changes, I'll just diff the data and broadcast change events upwards.

Translating these internal events into public Firebase events will be by no means easy, but certainly easier than what's there now.

Deprecations

No more Simple Login. The namespace will change so MockFirebase is exported directly.

Miscellaneous

I highly doubt onDisconnect (#62) will make it into 1.0.

bendrucker commented 9 years ago

cc @davideast @jwngr

bendrucker commented 9 years ago

Made a bunch of progress on thinking through how to do the data flow to cut a lot of the complexity out of the event system.

Read 1bb92da6872dab98a17ca4d92eae1f7f6dc1cb54 if you're interested. Otherwise, hoping to have more concrete stuff to show off in the coming weeks.

bendrucker commented 9 years ago

cc @urish @jamestalmage as well

Super happy with how things are turning out thus far. Behavior's much more consistent with real Firebase, most importantly the following:

ref.child('foo') !== ref.child('foo')
jamestalmage commented 9 years ago

RE: Endpoint Caching

From what I can tell, endpoint caching in firebase is achievable without WeakMap. Data without any listeners (on the current node, or any parent node), is immediately discarded.

Add Listeners Before Write:

// Assuming no listeners yet in this client session
var ref = new Firebase("path/to/node"); 
ref.on("value", myListeners);
ref.set({/** some data **/});
/* myListener has been called synchronously with the set data 
   (which will be corrected if set fails on server) */

Write before adding listeners:

// Assuming no listeners yet in this client session
var ref = new Firebase("path/to/node"); 
ref.set({/** some data **/});
ref.on("value", myListeners);
/* myListener has NOT been called, and will be called asynchronously with
   data synced from the server (which will be whatever we passed to `set()` 
   assuming the write was permitted) */

Writing to parent after listener is added to child:

// Assuming no listeners yet in this client session
var parent = new Firebase("path/to/node/");
var child = parent.child("child/path");
child.on("value", childListener);
parent.set({/** some data **/});
/* childListener is called synchronously with subtree of set data */

Add listener to a child after listening parent has synced data:

// Assuming no listeners yet in this client session
var parent = new Firebase("path/to/node/");
parent.on("value", parentListener);
/* Sometime later after data syncs with server  (i.e. parentListener has already been called once). */
var child = parent.child("child/path");
child.on("value", childListener);
/* childListener is called synchronously with cached data */

The caching algorithm when calling ref.set() on a given node seems to be:

  1. If the client currently has any value listeners on this node then cache the data (end).
  2. If the client currently has any value listeners on any parent of this node then cache the data (end).
  3. If the client currently has any value listeners on any child node, do not cache this node, but traverse to listening children and cache accordingly.

I have confirmed all of this via manual testing. Other listener types (child_added, child_changed, etc.) also have impacts on caching, but I don't recall the exact symantics.

Basically, firebase only caches data it has listeners for client side, and discards everything else (obviously if you are holding on to stale data via a reference to a DataSnapshot, that is not going to get garbage collected either - but that is to be expected).

Emulating this correctly for a testing environment could get tricky, but I think a proper API actually includes the notion of what data is "on the server" and what is "on the client":

var MockFirebase = require('mockfirebase');
var server = new MockFirebase.Server();
var Firebase = server.newClient('client1');   // inject this into tests in place of Firebase api
var Firebase2 = server.newClient('client2'); // each client instance maintains its own list of listeners, and it's own cache of data that has been synced

// api methods
server.flushWrites(); // Change the state of pending server side data from clientRef.set() calls
server.flushNotifications(); // Push changed states to any listening clients
server.flush(); // flushes writes and notifications (same as calling above methods sequentially).

server.flushWrites('client1'); // flush only a specific clients writes
server.disconnect('client1'); // put client1 in disconnected mode and trigger it's `onDisconnect` action

This initialization boilerplate is definitely more complex than what we have currently, and might not be needed in a lot of cases, so provide some wrapper that hides the details.

Eventually it would be pretty cool to extend it to parse Firebase rules:

var server = new MockFirebase.Server({'.read': /* and other rules */});
var admin = server.newClient('admin', {admin:true});
var Firebase = server.newClient('client1', {/** auth data**/});

I am aware that what I am proposing seems to add some complexity, but I think mimicking what happens in the real world (i.e. that the server and client really are separate entities with different copies of the data) will help tremendously in keeping the API's consistent.

In particular, I would really like Mockfirebase to accurately reflect which operations are going to end up causing listeners to be called synchronously vs asynchronously. We currently have the autoFlush option, but you don't have granularity of control over cached vs. non-cached (i.e. synchronous vs asyncronous).

bendrucker commented 9 years ago

First to address some confusion here. I'm referring to #67 which just means new Firebase('http://endpoint') === new Firebase('http://endpoint'). That's disabled by default but it's also done and super easy:

https://github.com/katowulf/mockfirebase/blob/fba9656fdee5af51788f9633c9cc9d04e0b543af/src/cache.js https://github.com/katowulf/mockfirebase/blob/fba9656fdee5af51788f9633c9cc9d04e0b543af/src/firebase.js#L27-L31

It's disabled by default because it's a memory leak. Every root reference can never be garbage collected until you manually delete it from the cache. And my original thought about WeakMap was wrong. WeakMap keys must be objects so it's useful for all kinds of lazy async behavior (like thenables) but not this.

As for the Firebase caching behavior, I'm interested in getting this right regardless of the number of people who care. That said, synchronously calling listeners when the cache is warm is just wrong. Caching is awesome, releasing zalgo isn't.

Happy to change my mind here if there's a good real-world case that can't be tested right now.

jamestalmage commented 9 years ago

I think new Firebase('http://endpoint') === new Firebase('http://endpoint') should always evaluate false. Both instances should share a common cache of the backing data.

That said, synchronously calling listeners when the cache is warm is just wrong

Could not agree more. Unfortunately, that is the behavior of firebase last time I checked currently. If they are willing to change the core Firebase API to a more sane behavior, great. But, like it or not, MockFirebase should emulate what Firebase does in every way possible.

bendrucker commented 9 years ago

Both instances should share a common cache of the backing data.

Currently (as in master), each reference maintains a ton of state. In v1, child references have no state whatsoever. The root (whether you explicitly construct it or not) stores all the state, namely:

So yes, you're correct that it would be better to store these three items centrally rather than cache the Firebase instance itself. It's also extra effort for a feature I have no use for.

But, like it or not, MockFirebase should emulate what Firebase does in every way possible.

I'm generally inclined to agree, but supporting this adds a ton of complexity in order to reproduce a very fixable bug. I'm also not sure whether it's even possible to get into a situation where you'd see this with MockFirebase.

bendrucker commented 9 years ago

Wrote up https://gist.github.com/bendrucker/1d2451c0933405e96a8c and sent it to support

Th͏e Da҉rk Pońy Lo͘r͠d HE ́C͡OM̴E̸S

katowulf commented 9 years ago

@mikelehen could I borrow your expertise on how caching works and your knowledge of how synchronous event calls are going to change in the future?

bendrucker commented 9 years ago

Michael's reply to my email:

This is known behavior. The intention was to raise events synchronously when possible, especially in the case of a local ref.set() call triggering events, as this can be very convenient. The fact that events are also triggered synchronously when we happen to have local data cached was kind of an accident, but to change that now would be a breaking change.

That said, we are planning to change our behavior to always raise all events asynchronously, but that won’t happen for a little while since it is a breaking change, so we’re planning to bundle it with other breaking changes we want to make.

So basically Zalgo will remain until the next major version.

jamestalmage commented 9 years ago

@mikelehen

we are planning to change our behavior to always raise all events asynchronously

That sounds like you are going to make ref.set() create events asynchronously as well.

A few thoughts on that, if that is the plan:

  1. Provide a way to tell if the event is being raised due to a local call to ref.set() or new server data. (See my comment on @bendrucker's gist linked above).
  2. Provide a way for libraries like angularFire to override whatever nextTick function you use so we can leverage Angular's digest cycle (and other similar solutions) to provide async behavior without releasing the thread and allowing excessive repainting in the browser.
  3. Your own internal implementation of nextTick should mimic the behavior of Angular's digest cycle or $timeout service. Basically: run as all the async operations in the queue without releasing the thread. (@bendrucker mentioned something about using closures nextTick implementation - I'm unfamiliar and it may already do this, so this point may be moot).
aj0strow commented 8 years ago

I highly doubt onDisconnect (#62) will make it into 1.0.

Oh. My unit tests need this. Why not have disconnect and reconnect methods much like the flush method to give sync testing control to the developer.