Adds optimized bulk operations to DataView

[Danielku15]

Resolves #571

This PR addresses the proposed changes of #571 to have more efficient bulk update capabilities in the Data View.

PR Checklist

• Changes implemented
• QUnit Tests Added
• Code Documentation updated
• Cypress Tests Added
• Wiki updated - Grab the updated version from https://github.com/Danielku15/SlickGrid-wiki/blob/feat/bulk-updates/DataView.md

Proposed Changes

1. Add/Insert/Delete operations which accept multiple items

The first change is to add plural versions of the main modification methods: addItems, updateItems, deleteItems
These operations use more optimized array updates and ensure the related index lookup updates + refresh are only triggered once.

2. Bulk Support for beginUpdate/endUpdate

The second change is to allow devs to indicate that bigger bulk updates are performed on the DataView. Internally this bulk update flag is then used at certain places to delay or optimize certain operations.

For now it is used to delay the delete operations to the endUpdate call which allows a 1 time reorganization of the internal items array and the index lookup table.

Alternative API approaches could be:

• Add a separate beginBulkUpdate / endBulkUpdate method pair.
• Accept an object updateHints as parameter to beginUpdate instead of a simple bool.
• Add a separate setUpdateHints comparable to setRefreshHints to set the bulk option.
• Re-use setRefreshHints to set the bulkUpdate option.

3. Usage of a Map polyfill for better performance

SlickGrid tries to be very backwards compatible with old browser. Using a Map instead of a simple object for object lookups boosts the performance further. On a basic test on chrome:

With object:

With the Map polyfill:

4. Update of DataView QUnit tests

They were quite outdated in regard of the latest behavior. I adopted the current behavior in the test to make them green.
In the old days it seems onRowsChanged was fired with the args.rows filled to the newly created rows after the related update.
The current version contains in args.rowsthe indexes of the rows before the update. e.g. if we had 3 rows before, and delete the last one, it signals now that rows 0,1 and 2 changed. Row 2 does not exist anymore. As I think the current approach is the desired one, I rather updated the tests than fixing the logic.

5. New Example Page and Cypress test

I added a new example page showing the significant performance difference on using the newly available operations. On top I added a Cypress test which also checks that the efficient version takes maximum 50% of the inefficient one.

[ghiscoding]

nice polyfill, I think you move this code into the slick.core.js so that it be used by other files, the core has these kind of util classes/fns so I think it's better there... and also expose it like the others and then you would call it with the dot instead new Slick.Map() (you can copy the structure of Slick.Event or the other ones in there)

The rest of the code looks quite clean, maybe just 1 other thing I could say is perhaps adding comments to some of the functions to explain what they do might be beneficial for future refactoring or understanding. I know the rest of the file is not well document but that doesn't mean we can't improve it, right? ;-)

[Danielku15]

Seems like you're faster with reviewing than I am with pinging you in the issue 😆

I think you move this code into the slick.core.js so that it be used by other files,

Sure, will do that. Adapting other areas will likely be beyond the scope of this PR.

1 other thing I could say is perhaps adding comments to some of the functions to explain what they do might be beneficial for future refactoring or understanding

Sure, can do that. I was trying to stick to the current code style and practices in the file so I left them out. As you might know: there are 2 kind of devs in the market: those who claim comments are bad and those who claim comments are good.

Those who claim they are bad argue that the code should be structured and written in a way that it is clear from reading it what it is doing. 😁 e.g. some comments explaining a workflow, can be replaced by slicing the overlal method into a series of methods/functions which have proper names.

I also rather like comments if they are meaningful.

[ghiscoding]

haha yeah I know but nowadays must developers use some kind of minification process, at least I do with WebPack, and so these comments get dropped from the prod build. If some dev aren't doing that then it's their problem, I would prefer to have more comments but yes if the methods have proper naming and are meaningful then no need to add comments, we are in agreement on that 😉

[ghiscoding]

For the testing side, I'm not sure that the unit tests are actually used but I did add GitHub Actions for Cypress E2E testing few months ago, and it's executed on every PR/merge. If you have time to add some, that would be nice, see the readme here for more info, but we would still accept the PR without it though, it's just a "nice to have" (so far I'm the only one who added some, gotta start somewhere hehe)

[Danielku15]

The tests seem to work fine. At the beginning I had some issue and they were red:
https://github.com/6pac/SlickGrid/runs/1742431746

After my fix they became green. As I wrote on the issue I will try to adopt some existing Dataview tests to ensure the data integrity with the new bulk updates. But I will try to add also some dedicated ones for the new operations and the bulk delete.

[ghiscoding]

The Cypress E2E tests are the only ones that get executed in each PR, they do serve a purpose as you found out ;)

There is another /tests folder but I never tried it and I think it wasn't fully working @6pac might be able to comment on that part.

In my libs Angular-Slickgrid and the others, I also have Jest unit tests but those are just for my libs, we don't have them in here. The Cypress E2E tests were a starting point, there's room for improvement

[Danielku15]

QUnit test for DataView updated regarding the current behavior (as documented in the PR description) and new tests for addItems, inserItems, updateItems and deleteItems added:

Also added a bunch of QUnit tests for the beginUpdate(true) cases. This should cover the biggest regression parts:

Unfortunately the Cypress tests seem to focus heavily on the existing examples. I will need to see if I can provide a specialized example cypress can use for verification.

[ghiscoding]

Cypress is for E2E testing and yes it is for any of the existing example html files, the unit tests that you fixed/added might be enough.

BTW, so I don't know how to run those unit tests but if we can run them with an npm scripts, we could add them to the GitHub Actions, right now it's only Cypress but I'd be happy to add these unit tests in the flow. We might need something like this SO Answer

[Danielku15]

To run the QUnit test you simply open the test pages in the browser. e.g. http://localhost:8080/tests/dataview/index.html
In the flavor they are currently maintained in SlickGrid they are not very suitable for CI integration but rather for human verification. For CI integration you would need to use a more CI suitable runner like Karma. This allows testing in a headless browser with failed test feedback similar to Cypress. 😉

https://karma-runner.github.io/latest/index.html
https://github.com/karma-runner/karma-qunit

I can give it a very quick try to get them running via npm script. I can adopt some stuff from my library.

Update: I gave it a quick try with karma but most of this codebase is simply too old. It depends on stone age old libs from the libs folder (old jQuery, old QUnit) and they were quite tweaked to run on the particular HTML pages and not in a test runner. This code would need a lot of modernization.

[ghiscoding]

no problem you can skip the integration, I was just curious to see if it's doable, at least we have Cypress integrated which makes sure the UI isn't broken, so we'll stick with that. Thanks for looking into it

[Danielku15]

Everything done and ready. Took a bit time to get accomodated with Cypress but now we have a new example and test for it.

[ghiscoding]

any particular reason why this new Map is not part of the other extend on the top

SlickGrid/slick.core.js

Lines 10 to 12 in 94871cb

	"Slick": {
	"Event": Event,
	"EventData": EventData,

I would prefer to see it there all in 1 extend if possible

[Danielku15]

I tried to explain it in the comment down on the place where it is registered.

The var Map is only defined down there, it will be undefined here and therefore "undefined" will be exported. I had it up here at the beginning but got undefined everywhere.

[ghiscoding]

or we can move all the other ones down there then, in some files they are defined that way.

[ghiscoding]

oops sorry I merged another PR touching slick.dataview.js and that created a conflict on your side. I moved the onSetItemsCalled from the end of the method to the beginning so that it gets triggered first (before the refresh kicks in).
Sorry about that.

<<<<<<< feat/bulk-updates
      idxById = new Slick.Map();
=======
      onSetItemsCalled.notify({ idProperty: objectIdProperty, itemCount: items.length }, null, self);
      idxById = {};
>>>>>>> master

I can approve your PR after you move the core Slick methods all at the bottom, we should have everything in 1 place, if it needs to be all at the bottom that is ok, as long as they are all together I would prefer. Thanks

[Danielku15]

No problem. I merged the latest master into my fork+branch and moved the exports like proposed to the bottom. I saw this was already the case for some files, so I made it consistent.

Just let me know if there is more we need.

[ghiscoding]

oh ok I didn't expect the move to the bottom for more other files but I guess it's ok for consistency.

@6pac I'm ok with the new changes now, I approved the PR, you can merge it whenever you want

[ghiscoding]

@6pac
Did you have a chance to review the PR? I'm ok with it, thanks mate

[6pac]

Looks good to me. Do you want a release?

[ghiscoding]

Sure that would be nice for the first release of the year 😉

[ghiscoding]

@Danielku15
I want to thank you again for your PR, it is really well organized, well documented and well tested. I also just tried the example, after retrieving latest code and wow the bulk change really does make a big difference (200ms vs 1100ms).

So thanks again for your PR, if every PR would be as great as yours, it would be so easy to maintain this lib (I'm trying hard to make my PRs good as well but I must say, you've even beat me 😉 ) and in case you didn't see it, we (Ben) pushed a new version to npm as well. Feel free to contribute more PR like this in the future ;) 🚀

Thanks to @6pac also for maintaining the lib since few years already 😄

Cheers 🍺

[Danielku15]

@ghiscoding You're welcome. It was fun implementing it. I am also maintaining an open source library and I just practiced the work style I would like my contributors to have too 😉 Seeing the codebase in detail, it was even tempting to maybe translate SlickGrid fully to TypeScript using a more modern and maybe also more independent (e.g. eliminate jQuery) codebase. I have seen for my project that such a more commonly used project infrastructure also attracts more developers to contribute. It makes the whole development more smooth in the JavaScript ecosystem.

Unfortunately my time does not permit it to work more actively on other open source projects like this one. This change was mainly driven by our needs from the company I work in. But I still hope I can maybe contribute also in future some more things.

[6pac]

Out of curiosity, @Danielku15, do you have any opinions as to what a good alternative to jQuery would be? The ground has shifted a lot - jQuery originally was used primarily for cross-browser compatibility, it was almost a wrapper. However its role now is much more focussed on easy DOM manipulation and javascript 'sugar'.

[ghiscoding]

@Danielku15

Seeing the codebase in detail, it was even tempting to maybe translate SlickGrid fully to TypeScript

My libs are using SlickGrid (Angular-Slickgrid, Aurelia-Slickgrid, Slickgrid-Universal) and I wrote them all in TypeScript and I added every single bit of TS Types/Interfaces for SlickGrid, for example slickNamespace.interface.ts, slickGrid.interface.ts and slickDataView.interface.ts, so no need to rewrite them all, I just never had the time to rewrite SlickGrid itself in TypeScript, I have a lot to maintain myself

@ 6pac

do you have any opinions as to what a good alternative to jQuery would be?

Plain vanilla JavaScript is now the best alternative, a lot of the code structure and mentality of jQuery went into the new ES6 syntax, for example we now have document.querySelector('.my-class') or document.querySelectorAll('.my-class') for multiple, the result is not the same as jQuery but close to. There are things that might be a bit harder to replace in SlickGrid though, but the best thing would be to slowly replace jQuery wherever you can (for example we can start rewriting all the plugins to vanilla JS, keep the DataView and SlickGrid to the last)

[Danielku15]

As you indicate, you do not really need jQuery for most of the things nowadays, Browsers have most things built-in. For old-browser compatibility you can then rather use mini-wrappers only for the needed functionality. Like I did it for the Map class. (http://youmightnotneedjquery.com/)

Especially when it comes to modern projects using Angular, React etc. these people are happy if you do not depend on jQuery or bootstrap. They might be targeting a different library stack. In my library I follow the approach that I just have a thin jQuery wrapper which maps down to simple class methods, this wrapper is only added as jQuery plugin, if it is detected that jQuery is included in the browser.

[6pac]

Just for the record, I'm still dealing with browsers that are ES6 incompatible (a lot of my government clients still use IE11!). However I suppose there are probably shims for that.

I agree that jQuery will soon be superseded by vanilla JS - I'm just not sure we are quite there yet!

And I must play with typescript. I have to say though, I just simply have no urge to strongly type javascript - I like it just the way it is. It is one of my favourite languages. I get my strong typing fix with C#.
However I suppose statistically, it may decrease errors, especially in teams.

[ghiscoding]

However I suppose statistically, it may decrease errors, especially in teams.

That's the biggest benefit and if you come from C# world, then TypeScript (TS) is not that far off, they are written by the same company after all (Microsoft), the only big difference between the 2 is that in TypeScript Type is on the right like an object (because of JS syntax, there's just no way to put the type on the left), the rest is very similar and I caught ton of errors with TS, I fixed a few of these errors by pushing back PRs in this lib ;)

The thing is that now I use Slickgrid-Universal in Salesforce and that plarform uses plain ES6 JS and man I miss TS so much when I code on that platform, I'm missing all the VSCode intellisense that TS brings, I always have to refer to my Wiki docs because I don't have intellisense there, sux really because I love that part too in TS world.

Just for the record, I'm still dealing with browsers that are ES6 incompatible (a lot of my government clients still use IE11!).

Yeah that's the only blocker but writing it in TypeScript we could still target ES5, CommonJS, UMD, so we can still get it to work. For example I use the UMD output bundle in Salesforce. The main difference is that if we rewrite SlickGrid in TS, we would have to change how we import the lib in our project, but it's not a big deal