BUG: AgGrid.__setattr__ breaks class properties (fix included)

[othalan]

The current implementation of AgGrid.__setattr__ breaks class properties (@property) because it bypasses normal setattr functionality built into python. Class properties cannot be set.

The following diff shows how to fix the bug:

diff --git a/justpy/gridcomponents.py b/justpy/gridcomponents.py
index 1055522..02faeb5 100644
--- a/justpy/gridcomponents.py
+++ b/justpy/gridcomponents.py
@@ -56,7 +56,7 @@ class AgGrid(JustpyBaseComponent):
             else:
                 self.__dict__[key] = value
         else:
-            self.__dict__[key] = value
+            super().__setattr__(key, value)

     def on(self, event_type, func):
         # https://www.ag-grid.com/javascript-grid-events/

A more robust implementation (also the python recommended implementation) would be to turn AgGrid.options into a class property, eliminating the need for setattr and improving the quality of the code. The change simplifies the code and looks like this:

diff --git a/justpy/gridcomponents.py b/justpy/gridcomponents.py
index 1055522..a38b111 100644
--- a/justpy/gridcomponents.py
+++ b/justpy/gridcomponents.py
@@ -40,8 +40,6 @@ class AgGrid(JustpyBaseComponent):
         for k, v in kwargs.items():
             self.__setattr__(k,v)
         self.allowed_events = []
-        if type(self.options) != Dict:
-            self.options = Dict(self.options)
         for com in ['a', 'add_to']:
             if com in kwargs.keys():
                 kwargs[com].add_component(self)
@@ -49,14 +47,19 @@ class AgGrid(JustpyBaseComponent):
     def __repr__(self):
         return f'{self.__class__.__name__}(id: {self.id}, vue_type: {self.vue_type}, Grid options: {self.options})'

-    def __setattr__(self, key, value):
-        if key == 'options':
-            if isinstance(value, str):
-                self.load_json(value)
-            else:
-                self.__dict__[key] = value
+    @property
+    def options(self):
+        return self._options
+
+    @options.setter
+    def options(self, value):
+        if isinstance(value, str):
+            self.load_json(value)
         else:
-            self.__dict__[key] = value
+            if not isinstance(value, Dict):
+                self._options = Dict(self._options)
+            else:
+                self._options = value

     def on(self, event_type, func):
         # https://www.ag-grid.com/javascript-grid-events/

I personally recommend the second fix (moving options into a class property) as it improves the overall code quality, however both versions of the above code fix this bug adequately.

Both diffs above are against the latest code base, v0.1.5. Let me know if you want a pull request.

[othalan]

Here is test code for this bug and to verify the fix:

import justpy as jp

class TestAgGrid(jp.AgGrid):
    _test_prop_value = 1

    @property
    def test_prop_double(self):
        return self._test_prop_value

    @test_prop_double.setter
    def test_prop_double(self, value):
        # Double the input value so that this is a non-trivial test case
        print(f'setting test property: {value} * 2 => {value * 2}')
        self._test_prop_value = value * 2

wp = jp.WebPage()
grid = TestAgGrid(a=wp)

assert grid.test_prop_double == 1 # Verify initial condition

# Set a new value to the property
grid.test_prop_double = 2

# Verify the property was set
if grid.test_prop_double == 1:
    print('BUG: Test property not set')
elif grid.test_prop_double == 4:
    print('FIXED: Test Property set as expected!')
else:
    print('Unknown BUG in test code')

[elimintz]

Thanks! This was a bug also in HighCharts. I fixed both.