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

Add actsAsOrderedSet #84

Closed
wants to merge 1 commit into from
Closed

Conversation

opsb
Copy link
Contributor

@opsb opsb commented Feb 3, 2015

@dgeb this is still a work in progress, I've made the updates to orbit.js and the tests look good. I had a quick go at integrating it with ember-orbit in an app I'm working on but it looks like I'll need to spend some time looking at how ember-orbit and orbit.js integrate tomorrow. I've put this up in case you get a chance to review.

@dgeb
Copy link
Member

dgeb commented Feb 7, 2015

We may want to change the flag from ordered to actsAsOrderedSet, since it's mutually exclusive from actsAsSet, and yet both are modeling set behavior. This will also clarify that any such relationships must be PUT as a whole data set for JSON API (and other sources, I imagine).

@opsb
Copy link
Contributor Author

opsb commented Feb 7, 2015

@dgeb yeah, that does seem more accurate. I'm hoping to get this tied up this weekend (need to do ember-orbit and check everything against an app).

Could you give me an overview of how actsAsSet actually works with ember-orbit? I can see that orbit.js only allows updateLink to be used by actsAsSet hasManys but I can't see how ember-orbit would switch between using addLink and updateLink (I'd expect https://github.com/orbitjs/ember-orbit/blob/master/lib/ember-orbit/links/has-many-array.js to use updateLink for actsAsSet).

@gnarf
Copy link
Contributor

gnarf commented Feb 7, 2015

Is there any reason to not use an array for all hasMany and default to always being ordered - things that don't care about order can feel free to ignore the index?

@opsb
Copy link
Contributor Author

opsb commented Feb 7, 2015

@gnarf it's a good question, we'd need to add some more explicit handling of dupes (using a hash avoids having to worry about this but I'll have to add it anyway for actsAsOrderedSet). You'll see in this PR I've added the index as an additional optional arg to addLink and removeLink. All in all though, always using an orderedSet does seem simpler.

@gnarf
Copy link
Contributor

gnarf commented Feb 7, 2015

The point about dupes is an important one...

@opsb
Copy link
Contributor Author

opsb commented Feb 7, 2015

Been looking at firebase and they make a great point about arrays being difficult to update in a distributed environment, it's a case of last write wins when really you want to merge the changes together. I'm now considering switching to using a priority field on each of the items in the list. This has it's own problems, i.e. you can end up with multiple items in the list with the same priority but at least you don't lose items in the list. It is resolvable however because you can stretch the list out and give the overlapping items unique priorities at a later time.

@dgeb
Copy link
Member

dgeb commented Feb 7, 2015

they make a great point about arrays being difficult to update in a distributed environment, it's a case of last write wins when really you want to merge the changes together.

That's why I think this is only acceptable if the entire array is updated as a set, and why I wanted this flag to be actsAsOrderedSet. If you really want merging instead of replacement, then I agree that some sort of priority system on a normal collection makes sense (although it is non-trivial too).

@dgeb
Copy link
Member

dgeb commented Feb 12, 2015

@opsb are you still pursuing actsAsOrderedSet in this branch or have you abandoned it in favor of your Firebase source? Fine to keep this open for now if everything's still a WIP - just asking.

@opsb
Copy link
Contributor Author

opsb commented Feb 12, 2015

@dgeb I'm circling back round to it after I've done the integration with Firebase. Because I'm integrating two unfamiliar techs (orbit.js and firebase) I wanted to see the big picture before I finished off with this branch. I'm still undecided about the best way to manage arrays for my app but I will finish off this PR either way.

@dgeb
Copy link
Member

dgeb commented Feb 12, 2015

@opsb makes sense - thanks 👍

@opsb opsb force-pushed the ordered_collections branch 7 times, most recently from 7f83973 to b1a39fb Compare February 27, 2015 04:12
@opsb opsb changed the title wip - adding ordered collections Add actsAsOrderedSet Feb 27, 2015
@opsb
Copy link
Contributor Author

opsb commented Feb 27, 2015

@dgeb this PR is ready to go now. Let me know if there's anything you want me to amend.

@opsb
Copy link
Contributor Author

opsb commented Feb 27, 2015

The corresponding PR for ember-orbit is at orbitjs/ember-orbit#56

@opsb
Copy link
Contributor Author

opsb commented Mar 22, 2015

Rebased against master

@gnarf
Copy link
Contributor

gnarf commented Apr 21, 2015

It's kinda sad that we never got this in.... It needs a rebase again 😦

I thought we had and was all telling @mzgoddard about it as if it were a real thing 😄

@opsb
Copy link
Contributor Author

opsb commented Apr 21, 2015

I'm a little reticent now. I came across issues with old operations overwriting newer ones when using memory-source with a remote source. Currently there isn't any mechanism in orbit to establish which operation is newer so there's no way to know which one to prefer. This is a fairly thorny topic so I think we'll have to park it for now.

@gnarf
Copy link
Contributor

gnarf commented Aug 30, 2015

Should we close this PR @opsb , will you update it / rebase it / work on it?

@opsb
Copy link
Contributor Author

opsb commented Aug 30, 2015

I've been giving this some thought lately actually. The original problem was that there was no way to tell if an operation had been applied to an array or not. The operation relies on the index positioning so if an array is modified after the original operation has been applied then if the operation arrives at the source again (which is relatively common) then you can easily get stuck in infinite loops with two operations churning on the arrays. Now there's a transaction log however it's possible to check if a transaction has been applied already and stop the bad sequence. That said, there's obviously been a fair few changes since this PR was put together and it's probably better to put it together again from scratch using this one as inspiration.

@gnarf
Copy link
Contributor

gnarf commented Aug 30, 2015

Alrighty - going to close this then!

@gnarf gnarf closed this Aug 30, 2015
@opsb opsb deleted the ordered_collections branch June 10, 2016 22:17
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

Successfully merging this pull request may close these issues.

None yet

3 participants