These are chat archives for orbitjs/orbit.js

6th
Mar 2016
Dan Gebhardt
@dgeb
Mar 06 2016 09:26
@opsb please review orbitjs/orbit.js#266
it is green and I think ready to merge with rethink
tomorrow I'll get to work on fixing up the jsonapi source and its tests
Oliver Searle-Barnes
@opsb
Mar 06 2016 10:46
@dgeb I gave it a once over and it looks like a huge improvement!
Both the transform-builder and the higher level ops resolve really well.
Oliver Searle-Barnes
@opsb
Mar 06 2016 10:51

I’m obviously a little apprehensive about the removal of the liveQueries, I’d like to at least have a plan in place here. I see a couple of options

1) Make RxJS a dev only dependency and feature flag liveQueries
2) Move the liveQueries to a companion project, orbit-realtime

(1) would obviously be my preferred option but it really depends on your perception of the level of resistance to RxJS in the ember community. If including it, even as a non-requirement is going to be a red flag for some people then (2) would probably make sense. Using RxJS saves a lot of effort so I think while I’m working on the initial implementation it makes sense to continue using it. Potentially we could fold orbit-realtime back into orbit.js once it’s stabilised and we’ve had a chance to think about a non RxJS Observable implementation.

Dan Gebhardt
@dgeb
Mar 06 2016 15:15
@opsb morning!
thanks for the review
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:15
morning!
looks like you had a long day
Dan Gebhardt
@dgeb
Mar 06 2016 15:15
yeah, trying to make a push
I share your concerns / questions about RxJS - feels like a big TBD at this point
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:16
I’m just a little nervous about it going out of a sync if we have a long running feature branch
it’s possibly isolated enough to not be a big problem, just wary
Dan Gebhardt
@dgeb
Mar 06 2016 15:18
wary of what going out of sync?
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:18
the liveQueries work
Dan Gebhardt
@dgeb
Mar 06 2016 15:18
oh sure
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:19
I think a lot rests on how you think the ember community perceives RxJS
Dan Gebhardt
@dgeb
Mar 06 2016 15:20
well, Alex Matchneer really tried for a while to get RxJS to catch on in the community
before writing ember-concurrency
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:20
hmm
I saw that last week, haven’t reviewed yet
Dan Gebhardt
@dgeb
Mar 06 2016 15:20
it does work with Observables too
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:21
interesting
Dan Gebhardt
@dgeb
Mar 06 2016 15:21
so it's not an alternative exactly
it does solve a lot of the same problems though
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:21
right
Dan Gebhardt
@dgeb
Mar 06 2016 15:21
but it's not appropriate in orbit
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:21
right
Dan Gebhardt
@dgeb
Mar 06 2016 15:21
I like the Observable concept
and orbit could be the gateway for them into ember
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:22
yeah, the big win for me is that it models chains of Observables
right
Dan Gebhardt
@dgeb
Mar 06 2016 15:22
I am fine moving RxJS back in for expediency
we're not at 1.0 yet
and we have room to waver
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:23
in the short term I think it’s the only option
Dan Gebhardt
@dgeb
Mar 06 2016 15:23
yeah, as in today :P
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:23
I don’t want to be worrying about the Observable implementation when I could be working on optimistic coordination etc.
Dan Gebhardt
@dgeb
Mar 06 2016 15:23
exactly
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:23
but yeah, once it’s working, we can figure out what’s most appropriate
Dan Gebhardt
@dgeb
Mar 06 2016 15:24
:+1:
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:24
If we make it a dev dependency user’s of orbit.js can basically ignore it for now anyway
Dan Gebhardt
@dgeb
Mar 06 2016 15:25
but it will be needed to use orbit, no?
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:25
only for liveQueries, it’s referenced as a global so there’s no imports
if liveQueries aren’t actually used then I don’t think it’s an issue
Dan Gebhardt
@dgeb
Mar 06 2016 15:26
I suppose everyone will want liveQueries - let me just move it back and refactor it for now
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:27
ok, I guess we’ll hear about it if it really upsets people
There doesn’t seem to be any awareness of it with the developers I’ve spoken to though
well, ember developers anyway, the node guys all know about it
Dan Gebhardt
@dgeb
Mar 06 2016 15:28
I'm concerned about the ember universe getting isolated tbh
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:28
yeah, I do find that aspect of the community a little frustrating
NIH rules at the moment
Dan Gebhardt
@dgeb
Mar 06 2016 15:28
the concepts in ember-concurrency shouldn't be as revolutionary as people are acting for instance
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:29
right
Dan Gebhardt
@dgeb
Mar 06 2016 15:29
I will try to change the NIH culture
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:29
There are a lot of very good ideas in the wider community now
Dan Gebhardt
@dgeb
Mar 06 2016 15:29
right
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:29
I do feel like ember is a long way behind, particular in terms of the data flow story
but hopefully we’ll fix that :)
Dan Gebhardt
@dgeb
Mar 06 2016 15:29
indeed :)
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:30
ok, so do you want me to take care of adding liveQueries back in?
you mentioned refactoring, I’m not sure any’s required is it?
Dan Gebhardt
@dgeb
Mar 06 2016 15:30
well, all the operators need to use the new operations
that's all
I'd appreciate you adding them back in and refactoring as needed
and I will focus on the jsonapisource
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:34
hmm, I think that should be pretty straight forward actually, they always use operationType() e.g.
        switch (operationType(operation)) {
          case 'addToHasMany': return addRecordOperation(record);
          case 'removeFromHasMany': return removeRecordOperation(record);
          default: throw new Error(`relatedRecords operator does not support: ${operationType(operation)}`);
        }
anyway, yeah, I’ll take care of that
Dan Gebhardt
@dgeb
Mar 06 2016 15:35
thanks
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:35
Regarding querying the sources, what do you think to
const fetchResults = source.fetch(query);

fetchResults.transforms;
fetchResults.lastTransform;

const subscription = source.subscribe(query); // for liveQueries
So, keeping an object for the results, as we were with QueryResult
Dan Gebhardt
@dgeb
Mar 06 2016 15:35
up above, we'll want to use {op: 'addRecord', record} instead of addRecordOperation - it's easier to construct the ops now
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:36
right
so there’s no path I take it?
Dan Gebhardt
@dgeb
Mar 06 2016 15:36
right
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:36
would be redundent I guess
nice
yeah, that really does look better (the higher level ops)
Dan Gebhardt
@dgeb
Mar 06 2016 15:37
yes, I should better document them
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:37
so, thoughts on that last gist?
Dan Gebhardt
@dgeb
Mar 06 2016 15:37
so fetch is not bad!
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:37
fetchResults.lastTransform seems to be the important one
yeah, woke up this morning and it just seemed like the right word
Dan Gebhardt
@dgeb
Mar 06 2016 15:38
yeah let's go with fetch
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:38
ok
Dan Gebhardt
@dgeb
Mar 06 2016 15:39
but can it just return a promisified transform or array of transforms?
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:39
yeah, it could be either of those
Dan Gebhardt
@dgeb
Mar 06 2016 15:40
feels like we've had this discussion :P
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:40
which is why I thought fetchResults might be handy ;) makes it easy to change our minds later
yeah, I’m just confirming really, we went backwards and forwards a bit
I think it’s probably best to just press forwards with that approach
Just wanted to make sure you hadn’t had any seconds thoughts
It is possible that we may want to include meta data with the fetch results
Dan Gebhardt
@dgeb
Mar 06 2016 15:42
that's true
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:42
If we return an object we’re hedging our bets
Dan Gebhardt
@dgeb
Mar 06 2016 15:42
should the metadata go on Transform?
such that every Transform can have metadata
about its source et al
there's a nice symmetry to that with transform() too
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:44
There’s definitely times when that will be useful (rethinkdb will include “timestamps” which can be used to check synchronisation for instance)
Dan Gebhardt
@dgeb
Mar 06 2016 15:44
right
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:44
I can see there being meta data for the “fetch” itself though
(in addition)
Dan Gebhardt
@dgeb
Mar 06 2016 15:44
ok
we can go with FetchResult
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:46
you’ve got me wondering about symmetry with transform() now, but I think we better back away ;)
we can tweak later
Dan Gebhardt
@dgeb
Mar 06 2016 15:47
well, why not start simple though?
both transform() and fetch() can return a single Transform
and we can see if we hit any bumpy patches
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:48
ok, can’t see any reason why not, let’s go with that for now
Dan Gebhardt
@dgeb
Mar 06 2016 15:49
we've been burned by adding unnecessary abstractions in the past :P
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:49
agreed
Dan Gebhardt
@dgeb
Mar 06 2016 15:49
pointing the finger at myself here to be clear ;)
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:50
ha, I’m sure I’ve been complicit ;)
Dan Gebhardt
@dgeb
Mar 06 2016 15:50
alright, let's see where this takes us ... I like the name and the symmetry between transform and fetch
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:51
yeah, that’s nice
ok, I’ve got some things to take care of first but I’m planning on getting back to that change later on
Dan Gebhardt
@dgeb
Mar 06 2016 15:52
I was going to start by getting transforms working with JSONAPI again
but I could introduce fetch instead
do you have a rough schedule?
no pressure of course
just want to sync up
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:55
hmm
let me just review where I am
Dan Gebhardt
@dgeb
Mar 06 2016 15:56
sure, sounds good
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:56
I was adding QueryResult which will now be replaced by the change to fetch
So you could go ahead and pick that up
I was backing away from JSON API anyway
Dan Gebhardt
@dgeb
Mar 06 2016 15:57
ok, sure thing
so I'll implement fetch in Fetchable I suppose
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:57
yeah, that would be great
Dan Gebhardt
@dgeb
Mar 06 2016 15:57
and work on jsonapi
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:57
I’ve got a little time this week, but I’m aware you’ll be moving faster than me, don’t want to be a blocker
Dan Gebhardt
@dgeb
Mar 06 2016 15:58
np! any help is appreciated
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:58
If you do that I can rebase off it and continue with the changes to the Store
Dan Gebhardt
@dgeb
Mar 06 2016 15:59
ok - is your refactor of Transformable going to happen along with those changes?
i.e. moving the queues
Oliver Searle-Barnes
@opsb
Mar 06 2016 15:59
yes, it’s all in the, slightly inappropriately named now, PR orbitjs/orbit.js#265
Dan Gebhardt
@dgeb
Mar 06 2016 16:00
gotcha - that's what I figured
:+1:
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:01
Think you could do a little PR just for the fetch change first?
Dan Gebhardt
@dgeb
Mar 06 2016 16:01
sure thing
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:01
great
thanks
Dan Gebhardt
@dgeb
Mar 06 2016 16:02
np
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:12
just reviewed ember-concurrency, as it’s built on Observable there should be nice some integration opportunities with liveQueries
Dan Gebhardt
@dgeb
Mar 06 2016 16:19
yeah, it's got a nice API - looking forward to trying it out
Dan Gebhardt
@dgeb
Mar 06 2016 16:35
@opsb what do you think about beforeQuery / query / queryFail, beforeTranform / transform / transformFail, beforeFetch / fetch / fetchFail events?
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:35
I have been thinking about that a little
what do we currently have?
just the query ones?
Dan Gebhardt
@dgeb
Mar 06 2016 16:36
yes
imo they should be consistent
they can lead to interesting composition patterns
but they can be a little expensive
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:36
yeah, I think it’s a good idea
right
what makes you say they (can be) expensive?
depending on the listeners?
Dan Gebhardt
@dgeb
Mar 06 2016 16:37
if they are in a hot path, they require creating several promises
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:38
they currenty use settle right?
I was thinking we could use emit instead
(which I think would make them cheaper in a couple of senses)
Dan Gebhardt
@dgeb
Mar 06 2016 16:39
yes, but using settle allows for the interesting composition patterns
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:39
I guess it allows the listener to decide whether it should be blocking or not
Dan Gebhardt
@dgeb
Mar 06 2016 16:39
right
well, let's leave them for now
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:40
yeah, think we need a concrete case to make that call
Dan Gebhardt
@dgeb
Mar 06 2016 16:40
yes
we should simply make sure that they are consistent across Fetchable, Queryable, and Transformable for now
Oliver Searle-Barnes
@opsb
Mar 06 2016 16:54
yeah, I think that would be useful, I did consider using those events instead of the promise returned from source.transform() for instance
Dan Gebhardt
@dgeb
Mar 06 2016 16:56
ok
I've always thought they have potential - will be interesting to see how they are used
I think I should probably move away from the mixin approach, so we can move to straight es classes
maybe follow the Evented.extend pattern instead
within sources
so they will call Transformable.extend(this) for instance
in the constructor
would be great to remove Orbit.Class
Oliver Searle-Barnes
@opsb
Mar 06 2016 17:15
yeah, that would be nice now we have bonefide ES classes
Oliver Searle-Barnes
@opsb
Mar 06 2016 17:38
If we follow the Evented.extend pattern it should be easy to convert them to decorators once they arrive.
Dan Gebhardt
@dgeb
Mar 06 2016 17:39
yeah, that's the right end goal
I'm already refactoring to Queryable.extend
Dan Gebhardt
@dgeb
Mar 06 2016 17:51
@opsb orbitjs/orbit.js#267
Oliver Searle-Barnes
@opsb
Mar 06 2016 17:52
that was fast! ok, reviewing
Dan Gebhardt
@dgeb
Mar 06 2016 17:52
should be pretty uncontroversial :)
Oliver Searle-Barnes
@opsb
Mar 06 2016 17:55
yep, I added a couple of code golfing comments but LGTM
Dan Gebhardt
@dgeb
Mar 06 2016 17:55
thanks - I'll clean those up and merge
Oliver Searle-Barnes
@opsb
Mar 06 2016 17:58
added one more for the golfing
Promise.prototype.tap would be really useful in some of those promise heavy sections. I added a monkey patch for the tests to use, not something I’d do outside of the test env though.
Dan Gebhardt
@dgeb
Mar 06 2016 18:05
yeah
ok, just fixed those up - please merge when ready
I'll steer clear of Transformable for now, since that's in your WIP
and I'll get back to jsonapi after some lunch
Oliver Searle-Barnes
@opsb
Mar 06 2016 18:13
ok, merged
Dan Gebhardt
@dgeb
Mar 06 2016 18:37
thanks :)
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:02
@dgeb I’ve closed the blocking-coordinator PR and opened orbitjs/orbit.js#268
I’ve consolidated things, it’s green up until the final commit which introduces an integration test that includes the JSON API soure and also the blocking behaviour in the Store.
I’m going to take a break now (I’ll still be around if you have any questions). My intention is to pick it up in the evenings this week but if it becomes a blocker then it should be in a form where you can pick it up fairly easily now.
Note I’ve cleared out the blocking connectors etc. as they’re not necessary in the final approach (blocking at the store).
Dan Gebhardt
@dgeb
Mar 06 2016 20:08
@opsb ok, thanks for the update
so you're feeling pretty good about the commits up until the last wip?
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:13
It’s largely removing the transform-connector and related integration tests (most of them) and then introducing new components that are used by the final wip commit
So perhaps we could merge them in
Dan Gebhardt
@dgeb
Mar 06 2016 20:15
are we sure about removing the connectors? they still seem appealing for ad hoc one way connections between sources (i.e. that don't require confirmation)
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:16
In their current form they don’t really offer much over
source.on(’transform’, t => target.transform(t));
Dan Gebhardt
@dgeb
Mar 06 2016 20:17
I agree - not too much
there is the blocking flag
plus activation / deactivation
and a shouldTransform method that can be overridden
they are very thin
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:19
yeah, am I thinking, but they seem like features without a purpose at the moment
and shouldTransform is very easily emulated using the above pattern
Dan Gebhardt
@dgeb
Mar 06 2016 20:20
that's true
and so is blocking
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:20
yeah
Dan Gebhardt
@dgeb
Mar 06 2016 20:21
although that might not be obvious with the t => target.transform(t) form
that that is blocking I mean
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:21
true
if we’re moving blocking to the store though, I’m not sure that we should be concerned with that
Dan Gebhardt
@dgeb
Mar 06 2016 20:23
we still have promise-aware events like beforeTransform / transform / afterTransform
and those can block
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:23
we could introduce helpers
source.on(’transform’, t => blocking(target.transform(t)));
source.on(’transform’, t => nonBlocking(target.transform(t)));
Dan Gebhardt
@dgeb
Mar 06 2016 20:24
true
it seems worthwhile to keep the connectors if they can have a queue
because that's kind of critical if queues are off the sources themselves
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:25
I quite like that the queue is a standalone component in the scenario I’ve been putting together
Dan Gebhardt
@dgeb
Mar 06 2016 20:25
ok
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:25
I’ve always thought it would be nice to have a set of small components that we just chain together
more of a DSL I suppose
Dan Gebhardt
@dgeb
Mar 06 2016 20:26
yeah, that is appealing
so let's see how cleanly we could simulate blocking connectors with a queue
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:26
ok
I think it would be
source.on('transform', t => queue.add(t));
queue.on('transform', t => blocking(target.transform(t)));
Dan Gebhardt
@dgeb
Mar 06 2016 20:28
seems good :shipit:
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:28
ha
Dan Gebhardt
@dgeb
Mar 06 2016 20:29
I think people playing with low-level orbit connections should be able to handle that :P
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:29
ha
yeah, I think we can assume a little technical competence ;)
Dan Gebhardt
@dgeb
Mar 06 2016 20:30
yeah, I'm afraid that orbit originally looked too approachable and then people fell off a cliff
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:30
ha, yeah, I’m just landing ;)
Dan Gebhardt
@dgeb
Mar 06 2016 20:30
ha!
same
now we are creating something that is less of a black box at this level
but at the higher levels, like ember-orbit, it should be very friendly
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:31
yes, I’m keen to make it a pleasurable experience
ember-data falls short there

BTW, the previous line of thought was partly what led me to Observables, we could represent

source.on('transform', t => queue.add(t));
queue.on('transform', t => blocking(target.transform(t)));

as

source.transforms.queue.subscribe(target);
(even simpler)
Dan Gebhardt
@dgeb
Mar 06 2016 20:33
such magic :sparkles:
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:33
heh
Dan Gebhardt
@dgeb
Mar 06 2016 20:34
well, we could have source.transforms, source.fetches, and source.queries as observables
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:35
yes, there’s definitely scope for playing with those
Dan Gebhardt
@dgeb
Mar 06 2016 20:35
but it's probably either / or with promisified events or observables, right?
otherwise, interacting with one could influence the other
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:36
hmm
Dan Gebhardt
@dgeb
Mar 06 2016 20:36
but we could easily provide observables that wrap the events I suppose
there is some potential there
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:37
the Observable semantics are a superset(they’re stream aware), so it would probably be the other way around, but I think we could probably figure something out
Dan Gebhardt
@dgeb
Mar 06 2016 20:37
ok
Oliver Searle-Barnes
@opsb
Mar 06 2016 20:39
I think we’re fine to proceed with the existing events at the moment, the optimistic coordinator is where I suspect we’ll see a real advantage to Observables
(being able to cancel streams will be important there)
Dan Gebhardt
@dgeb
Mar 06 2016 20:39
that makes sense
Dan Gebhardt
@dgeb
Mar 06 2016 22:35
I'm marching through the classes and converting to ES classes now
soooo therapeutic :)
Oliver Searle-Barnes
@opsb
Mar 06 2016 23:13
ah, yes that will be very nice