Fix memory leak issue

[howdyAnkit]

Fixes issue for #2242

[netlify]

✅ Deploy Preview for jsonforms-examples ready!

Name	Link
🔨 Latest commit	b715395
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/6601a526304d7200088f1b73
😎 Deploy Preview	https://deploy-preview-2243--jsonforms-examples.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[howdyAnkit]

Hi @lucas-koehler please review and let me know if you have any suggestions or any changes required thanks

[lucas-koehler]

Hi @howdyAnkit thanks for your contribution ❤️ Please fix the merge conflict so we can run the full CI :)

[howdyAnkit]

Sorry missed it @lucas-koehler have pushed again fixing merge conflcts and it's again open for review. thanks for checking and mentioning

[sdirix]

I'm not familiar with pipes. Can you quickly explain for the change when and how now a rerender is triggered?

Will the table rerender when seemingly unrelated changes happen? For example a property outside the table is changed and a SHOW rule associated with the table now evaluates differently. Will the table be updated, i.e. shown/hidden then?

[howdyAnkit]

Hi @sdirix thanks for reviewing.

Can you quickly explain for the change when and how now a re-render is triggered?

The changes should be triggered normally as compared to previously the function was getting triggered aggressively because the function was being called on the html itself. but now since I have changed with Pipes. Pipes by themselves do not trigger re-renders or DOM updates. They perform transformations based on input data and are primarily used for display purposes and perform transformations on data without triggering re-renders.

Considering previous example where the function was called directly and it was detecting the changes on hover as-well instead now if there's any new change it will definitely trigger a re-render it's just that it will be triggered once and the no of time's you interact or the no of time it's been called. I'd still like to see if you have any examples that could possibly be a better way to validate the code instead of assumptions.

[howdyAnkit]

Hi @lucas-koehler and @sdirix any updates on the above. any inputs required from my end?

[sdirix]

Hi @howdyAnkit, we were busy with the 3.2 release in the last two weeks, therefore the late reply.

From what you explained, I think the approach is sound.

This is the scenario to be tested:

JSON Schema

{
  "type": "object",
  "properties": {
    "enabled": {
      "type": "boolean"
    },
    "people": {
      "type": "array",
      "items": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string"
          },
          "age": {
            "type": "number"
          }
        }
      }
    }
  }
}

Ui Schema

{
  "type": "VerticalLayout",
  "elements": [
    {
      "type": "Control",
      "scope": "#/properties/enabled",
      "label": "Is Enabled"
    },
    {
      "type": "Control",
      "scope": "#/properties/people",
      "label": "People",
      "rule": {
        "effect": "ENABLE",
        "condition": {
          "scope": "#/properties/enabled",
          "schema": {
            "const": true
          }
        }
      }
    }
  ]
}

The table should still be enabled or disabled based on the enabled property.

I would also recommend to rebase the PR on the latest state of master to potentially fix the seemingly unrelated test errors.

[howdyAnkit]

Hi @sdirix thanks for replying and getting back PFA working repo to validate the schema's given above (https://github.com/howdyAnkit/fix-2242). Everything looks working fine to me. i'll rebase the branch and push again and if any other changes required please let me know.

Thanks Ankit

[howdyAnkit]

Also I have added a last commit to include the pipe in module folder to make it work or else it will throw errors please do let me know if it needs to be added few other places aswell. Thanks for checking : )

[sdirix]

Can you check the failing Angular Material test cases?

The error message starts with

Failed: NG0302: The pipe 'getProps' could not be found in the 'TableRenderer' component. Verify that it is declared or imported in this module. Find more at https://angular.io/errors/NG0302

[howdyAnkit]

Hi @sdirix @lucas-koehler thanks for reviewing. did it passed this time?

[howdyAnkit]

I can still see it failing 💔. the only problem in my view was the missing import in module i'd like to hear you're suggestion if you have.

[coveralls]

coverage: 84.804% (-0.08%) from 84.881%
when pulling b715395 on howdyAnkit:fix-2242
into 24f4e5f on eclipsesource:master.

[howdyAnkit]

Thanks @sdirix for adding the imports and fixing this issue ❤️.