Ngx-datatable: Do not mutate objects

[shlomiassaf]

I'm submitting a ... (check one with "x")

[x] bug report => search github for a similar issue or PR before submitting
[x] feature request

Current behavior
The table mutate the objects / raws with an $$index property. code

Expected behavior
Do not mutate objects.

What is the motivation / use case for changing the behavior?
It is considered a bad practice to mutate objects sent by the user.
With today's modern language constructs it can be easily avoided.

The issues vary, here is a summery:

1. Impossible to handle immutable objects
2. Unintentional reflection of the $$index property
3. Usage conflict (the user or other 3rd party lib's might be using $$index

The user is not aware that this happens and this might create serious implications with the current implementation.

For example:

A user might use the ImmutableJS library or use Object.freeze/seal on the objects sent as rows, this will cause an exception by the library that can't handle these objects. (ImmutableJS is a quite common library)

If an object is sent to the table and then used to create an HTTP request the $$index property will be sent along with the request. This is unintentional reflection.

There are several solution to solve this, this is my preferred solution:

• Use WeakMap to store the index of every row, rows are automatically removed from the WeakMap instance when removed from the table (and no longer referenced by the user). Each table instance should have a WeakMap instance.
  It is possible to remove the row from the weekmap when removed from the table.

  // somewhere at construction: 
  this.idxMap = new WeakMap() 

  // set index:
  this.idxMap.set(row, rowIndex);


 // use this.idxMap.get to fetch the index.

This solution can't create conflicts, it will not reflect on object serialization and it will not mutate the object.

Other possible solutions:

• Use a Symbol instead of a property, symbol are guaranteed not to conflict with any other library.

  // somewhere at module's top: 
  IndexSymbol = Symbol('ngx-datatable RowIndex');

  // set:
  row[IndexSymbol] = rowIndex;

 // get
 row[IndexSymbol]

This solve any conflict and will not reflect in serialization but it will throw on immutable objects.

• Assign $$index with a property descriptor so the value is not enumerable.

  Object.defineProperty(row, '$$index', { value: rowIndex });

This does not solve usage conflicts and mutates the object, not good.

[amcdnl]

Agreed. We are discussing the subject over here already: https://github.com/swimlane/ngx-datatable/issues/253

[amcdnl]

Would you mind PR'n this? I would be happy to accept.

[shlomiassaf]

What path should be taken?

Do you have branchmark test that can assess the best approach.

Weakmap is the best solution since it solves everything but it needs to be tested performance wise

[amcdnl]

Weakmap would be my pref.

The sticky issue is rows changing via sort/etc.

[shlomiassaf]

I can't promise anything cause i'm overloaded but i'll try making a PR

[jeandat]

Hi everyone,

Very happy to find this issue and pull request. We are using @ngrx/store and so we are very interested by this fix.

It is my understanding you merged @shlomiassaf PR into the branch "mutations".
Would you be able to give us a roadmap regarding the deployment of this fix ?

And most importantly, could you push me in the right direction if I want to use this fix right now: what could I do ?

Thanks a lot.

[amcdnl]

I had to unmerge his changes due to regressions. It is very high on priority, there are many issues around performance and making this happen we need to be careful on which is why the delay.

[andrii-oleksyshyn]

Do you have any updates regarding this issue?

[jeandat]

Yeah would love an update too 😋

On Mon, Jul 10, 2017, 5:21 PM Andrii Oleksyshyn [e-mail]
wrote:

Do you have any updates regarding this issue?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/swimlane/ngx-datatable/issues/610#issuecomment-314139571,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACptLtS_OOmYIgUcMk7wrovGX11LE48Mks5sMkGAgaJpZM4MjL0H
.

[amcdnl]

Planning on this for next release, probably be in July sometime.

[cime]

Any update?

[amcdnl]

done in 10.0.0!!!!!