Primeng: Reimplement equals check without manipulating objects

[remmeier]

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

[x] bug report => Search github for a similar issue or PR before submitting
[ ] feature request => Please check if request is not on the roadmap already https://github.com/primefaces/primeng/wiki/Roadmap
[ ] support request => Please do not submit support request here, instead see http://forum.primefaces.org/viewforum.php?f=35

Current behavior

The domhandler in https://github.com/primefaces/primeng/blob/master/components/dom/domhandler.ts
make use of a _$visited flag to mark objects as being visited. There are two issues with that:

• a basic "equals" object that should be read-only modifies objects
• some of those _$visited markers are left on the object after the method returns

This _$visited lead to various problems:

• ngrx/store breaks as the immutability is violated (the developer tools have a problem with that)
• HTTP calls break as many servers will reject objects that have additional _$visited. So a custom filter needs to be implemented.

Expected behavior

Preferably the domhandler would not make use _$visited markers on the object. Or the least it should keep a list where it added it and remove them again when the method exists.

Minimal reproduction of the problem with instructions

just make use of one of the components making use of domHandler.equals, such as the auto complete component.

What is the motivation / use case for changing the behavior?

It severely affects the implemention of user interfaces.

Please tell us about your environment:

independent of that

• Angular version: 2.0.X

independent of that

• PrimeNG version: 2.0.X

2.1.4

• Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

all of them

• Language: [all | TypeScript X.X | ES6/7 | ES5]

independent

[remmeier]

potentially it should be possible to replace the implementation of that equals method

[yunuscolak]

the problem continues. When is it fixed?

[cezaraf]

the problem continues. When is it fixed?

[mnn]

Very annoying issue (not fixed in current version on npm). I'll have to filter data on several places to clean this mess, because it breaks detection of changes.

[mlakhara]

@remmeier Hi Remo, any workaround with this issue with ngrx/store ? I myself am working on creating a wrapper on p-datatable and need something to handle this.

[mmikeyy]

Extremely annoying issue indeed. Spent the afternoon trying to figure out where this new property was unexpectedly added to objects. I should have checked this thread earlier...

Fortunately, it's easy to fix as far as I can tell. The error in their code is that in the domhandler#equals method, they set this temporary property and later delete it. However, they forgot that the method has more than one exit points and not all of them take care of deleting this property, so the temporary property is deleted or not depending on where the method exits returning its result.

What I've done is

1- put the datatable source code from git in my project directory in folder primeng/datatable.
2- put the dom/domhandler source code from git in my project directory in folder primeng/dom
3- modify the imports at top of datatable to refer to primeng in my node_modules (replacing all .. with primeng/components), except for dom/domhandler, which must refer to the new one that was imported (fixed below).
4- fix the domhandler#equals method as shown below. See comments where I made changes
5- in your module, import the fixed datatable module instead of the primeng one.

    public equals(obj1: any, obj2: any): boolean {
        if (obj1 == null && obj2 == null) {
            return true;
        }
        if (obj1 == null || obj2 == null) {
            return false;
        }

        if (obj1 == obj2) {
            delete obj1._$visited; //****** this was here but I don't think it's needed as the property hasn't been added yet
            return true;
        }

        if (typeof obj1 == 'object' && typeof obj2 == 'object') {
            obj1._$visited = true;
            for (var p in obj1) {
                if (p === "_$visited") continue;
                if (obj1.hasOwnProperty(p) !== obj2.hasOwnProperty(p)) {
                  delete obj1._$visited; // ******** added
                    return false;
                }

                switch (typeof (obj1[p])) {
                    case 'object':
                        if (obj1[p] && obj1[p]._$visited || !this.equals(obj1[p], obj2[p])) {
                          delete obj1._$visited; //**** added
                          return false;
                        }
                        break;

                    case 'function':
                        if (typeof (obj2[p]) == 'undefined' || (p != 'compare' && obj1[p].toString() != obj2[p].toString())) {
                          delete obj1._$visited; //***** added
                          return false;
                        }
                        break;


                    default:
                        if (obj1[p] != obj2[p]) {
                          delete obj1._$visited; //********* added
                          return false;
                        }
                        break;
                }
            }

            for (var p in obj2) {
                if (typeof (obj1[p]) == 'undefined') {
                  delete obj1._$visited; //******* added
                  return false;
                }
            }

            delete obj1._$visited; //***** they got this one right!
            return true;
        }

        return false;
    }

That's it. The temporary property has now disappeared from my datatable components.

[mlakhara]

@mmikeyy Thanks for sharing the fix with us.
@sudheerj Any chance this can land into the official repo? If required someone can send a PR for this.

[fatsu]

We encountered this issue on dataTable and were able to work around it by setting the "dataKey" prop.

[mmikeyy]

Note that the domhandler#equals corrected above has been moved to ObjectUtils#equalsByValue.

This method is conditionally called by ObjectUtils#equals depending on the number of parameters it receives.

The ObjectUtils#equalsByValue method is the only one in all primeng code that sets a _$visited property, so I'm confident that the fix above (not yet included in primeng) solves the issue. Circumventing the symptom by other means just leaves a ticking time bomb in the code, IMHO.

[cagataycivici]

If you use dataKey property then deepEquals is not used fixing this issue.

[mnn]

Is the fix merged (I don't see it in PRs)? Or why is this closed? I doubt that adding random fields to data (and thus breaking change detection) is good behavior...

[remmeier]

WTF? would you please properly implement that method or remove support for it altogher and throw an error.

[cagataycivici]

Please provide a PR to help us out then.

[cagataycivici]

Reopen

[cagataycivici]

My bad, I have accidentally closed it. I have asked my colleague to find an alternative implementation, note that we rathet not depend on loadashbor underscore just for this. Any feedback is welcome, I am +1 on replacing it.

[SIlver--]

Has there been a PR for this yet? @mmikeyy fixed the issue with his code above right?

[mlakhara]

Hi @cagataycivici , just checking if the solution by @mmikeyy is valid and does not affect anywhere else. If this works, then we should be able to merge this in the official repo. If you wish, I can create the PR with the exact fix (all credits to @mmikeyy though).

[Da13Harris]

@mmikeyy seems to have solved this, so I'm looking forward to an official PR. This causes us issues, because we're binding our PrimeNG DataTable to a FormArray. The unfortunate side effect is that all the contained FormGroup/FormControl items (which are passed along to an edit dialog) have this property on them, which then fail whenever Angular tries to iterate over them.

[bogacg]

Using dataKey seems to fix it, yet from documentation I get it as it's supposed to be used for performence, shouldn't be a requirement.

[cagataycivici]

p-dataTable is deprecated and will be removed in favor of the new p-table (aka TurboTable) of 5.1.0 so closing the issue. Please try the new p-table once 5.1.0 is released.