search

One-com / knockout-projections

Knockout.js observable arrays get smarter
5 stars 2 forks source link

SortBy: forward the underlying array when no sorting order is specified #7

Closed AdamWillden closed 9 years ago

AdamWillden commented 9 years ago

Potential fix for #5. See what you think.

It should be as quick if not quicker now as comparisons done using stateItems and their previousMappedValues rather than re-evaluating every single item every time we compare two items.

AdamWillden commented 9 years ago

To find an item we want to delete I make a mock stateItem which is then used to match against the existing stateItems. As such the mapping function gets called during the creation of this mock stateItem. This stateItem is never actually added to the stateItems and is immediately disposed of.

I added a mock true/false parameter to the mapping function when a it was being executed for this purpose. I couldn't think of a better name.

I can't see many reasons for people using this mock parameter, as I've mentioned in https://github.com/One-com/knockout-projections/issues/6#issuecomment-68996135 I asynchronously get the values for use in sorting, only when all values are 'resolved' do I add the values to the sort mapping result, in the meantime I display a status. Passing mock meant I don't wrongly update counters during the creation/destruction of mock objects.

sunesimonsen commented 9 years ago

Sorry - I didn't look at the pr yet. I'll try to allocate some time soon. Could you please rebase the branch on master and force push it - that would make my job a bit easier.

AdamWillden commented 9 years ago

No problem we all have our own things we need to do @sunesimonsen. I've rebased and force pushed it. Hopefully done it right.

AdamWillden commented 9 years ago

Hi @sunesimonsen I've just noticed that you've put a bunch of changes out and this is no longer up to date and has conflicts - I'll try to get round to resolving the conflicts as required.

Did you review this at all?

AdamWillden commented 9 years ago

Right I've sorted the conflicts as I wanted to incorporate your changes into this PR so I could use them! I've also squashed some commits together.

sunesimonsen commented 9 years ago

Hi Adam, I guess this pull request is just not bite sized, but that does not mean you deserve radio silence. I'm worried about the added complexity and the performance implications. I looked at the code - but it is a huge change. I checked out the code and I can see that some of the existing tests had gotten slower. I have a feeling that it is because of inputIndexMap but I don't know. I'm sorry, I know you put a lot of work into this, but this change is not something I'm going to pull.

We are moving the project to https://github.com/One-com/knockout-transformations, it is not completely ready, that's why I haven't notified you yet. The new project would enable you to override the sortBy transformation, by setting ko.observable.fn.sortBy = ko.transformations.fn.sortBy = . That might be the solution to have differences with the core library. I can understand if you lost patience with me, by I'm mainly looking out for One.com's interests and don't have a lot of time allocated to do so. A word of advise when creating pull request is to make them as small as possible and to the point. Maybe even do a few pull request that align the library with what you try to achieve, before doing the main pull request.

Kind regards Sune

AdamWillden commented 9 years ago

No problem. The change is so big because it is a fundamental change to the existing sortBy functionality. I expected the sortBy to chain like the other projections, respecting all those before it and allowing chained sortBy projections if necessary, but it didn't. In my eyes this brought it in line with the existing projections.

The time isn't wasted as I needed this functionality and I'll run with this and incorporate your changes as I can.

I hadn't compared the test speeds and I'm intrigued to see where it's slower and if I can improve on that. I know one place it would be slower is in adding items due to incrementing inputIndexes.

AdamWillden commented 9 years ago

FYI When simply comparing the the test outputs I'm not seeing any significant performance changes at all (when not including my new tests) - SortBy - 62 ms vs SortBy - 63 ms

Thanks for looking anyway.

sunesimonsen commented 9 years ago

Sorry I made a mistake of overlooking some of the new tests. Could you elaborate on:

I expected the sortBy to chain like the other projections, respecting all those before it and allowing chained sortBy projections if necessary, but it didn't. In my eyes this brought it in line with the existing projections.

I'm pretty happy with some of the changes in this pull request. I need a bit more time to review it. I know I had a lot of time already. But I didn't get to it.

sunesimonsen commented 9 years ago

I'm looking at onStructuralChange it seems like we are going from O(n * log n) to O(n * n), if that is the case I can't allow it. We are sorting upto 15000 items with the projection and that would have significant performance impact. Furthermore I still think this adds a lot of complexity. But I like some of the refactorings you did on handling the sorting keys, I might steel some of those. Another problem is that we moved the project, so I would have to manually merge it. So sorry I'm closing it again.

AdamWillden commented 9 years ago

Don't worry @sunesimonsen

I'll be glad if you manage to steel some of the ideas. The sortby re-factoring and the use of previousMappedKeys (rather than always re-evaluating the mapping) should be decent savers. I only noted those changes were possible once I came to understand the sortBy code else I would have split them out from the beginning. I wasn't doing the code with PRs in mind, least not at first.

I really wish i had an understanding of O(n * log n) to O(n * n) and how to figure it out but unfortunately I've never grasped it or seen any tutorials on how to figure it out. I understand what you're saying isn't great but I'm not sure how I'd go about rectifying it.

With regards to my statement, with two chained sortBy projections. If I change the sort order in the first it wouldn't reflect in the second output. In all fairness I don't have a use case (urrently) that requires two chained sortBy projections but I did expect my original sort order to be respected in that same kind of way.

I'll keep an eye on the new project and if I can see ways to improve mine to remove/minimise the performance hit I'll send a bite-sized PR.

I'm glad you looked over it enough to see changes you'd like to use.