Setting instance level attributes in Reflex as a way to set context data is a confusing API

[dylanjw]

Feature Request

Background: I want to start out by saying Im new to sockpuppet, but love this project. One thing though. I find setting instance level attributes in Reflexes as a way to update context data to be a confusing API

Related to issue #43, It would be nice to make the documentation more explicit that user added reflex attributes get propagated into the context data during refreshes.

I also find the existence of the context attribute in reflexes promotes this confusion. For myself and other uninitiated, setting context in a reflex instance has surprising behavior:

1. The context attribute is only populated with data after calling get_context_data(), and setting the context attribute overrides the data returned by get_context_data.

>>> reflex.context
{}
>>> reflex.get_context_data()
{'stimulus_reflex': True, 'view': <app.views.NewView object at 0x7ff32cdb36d0>}
>>> reflex.context
{'stimulus_reflex': True, 'view': <app.views.NewView object at 0x7ff32cdb36d0>}
>>> reflex.context = {"field2": "value2"}
>>> reflex.get_context_data()
{'field2': 'value2'}

2. Fields added to the context attribute in the reflex do not get passed to the template and view.

Possible solutions

Editing the documentation to better explain the API for changing context data would be a first step. A further improvement could be made to the overall API in one of two ways:

1. Make context "private" by prepending with a "_". Right now it looks like its part of the API and not something reserved for internal use.
2. Modify behavior where context data is pulled from reflex instance attributes, and instead update/merge with data from context.

class MyReflex(Reflex):
    def update(self):
        self.field = "data"

becomes

class MyReflex(Reflex):
    def update(self):
        self.context.field = "data"

I think option 2 is a better direction but it would be a breaking change. It would be ideal to release a minor version where context is pulled both from the context attribute and instance level attributes before phasing out the instance level attribute propagation in a major version.

Maybe I am confusing some of the intended API and behavior. Would love to hear others thoughts.

PR link

TBD

[jonathan-s]

Thank @dylanjw for opening a thoughtful issue. I agree that editing the documentation to become clearer is definitely a good first step. It should definitely be mentioned in clearer terms on the reflex section in the documentation. If you've got suggestions to where you would like to see it elsewhere I'm open to hear it.

What I had done earlier is to exclude "protected variables" > https://github.com/jonathan-s/django-sockpuppet/blob/master/sockpuppet/consumer.py#L237, however I had missed to add context to that. I guess it would be useful make a warning in the commandline if the user tries to add variables that are protected, mentioning context more clearly in the documentation should be done as well. Making context a private variable is also a good idea.

With that out of the way it should become less confusing for a user, or would you still find it confusing? Perhaps @nerdoc or @DamnedScholar have some thoughts as well?

[DamnedScholar]

What if Reflex.get_context_data() set to Reflex._original_context (removing the part where it aborts fetching the original context if there's already a context) and Reflex.context had a setter that set to Reflex._new_context? Then Reflex.context could have a getter that returns Reflex._original_context.update(self._new_context) and the original context would never get overridden on accident.

I do think it makes sense to only pass variables under context to the context. It's a little more wordy, less magical, more explicit. It also provides a cognitive separation between your product (what's going to the template) and your workspace (the Reflex and its variables).

[jonathan-s]

Making context a property in the first place would also make impossible to write over. So that would be another safe-guard.

[dylanjw]

I'd like to make another argument in support of namespacing passed context variables. Extending the reflex class with new methods/attributes has the potential to be a breaking change for someone with conflicting variable names in their project. While moving over to using context for passing variables is a bit of a pain now, it sets up the reflex class to be more easily extensible in the future.

[dylanjw]

I agree that editing the documentation to become clearer is definitely a good first step. It should definitely be mentioned in clearer terms on the reflex section in the documentation. If you've got suggestions to where you would like to see it elsewhere I'm open to hear it.

I can open a PR with some doc changes.

[jonathan-s]

I can open a PR with some doc changes.

That would be most welcome :)

[dylanjw]

I would also be into making the changes as @DamnedScholar suggested, to see what those could look like and keep the discussion going.

[nerdoc]

I don't know if I understood correctly. But this is one of the thins tat are unnecessarily complicated in Sockpuppet IMHO. First, at the template, you have to set the data-<variable>. and then it is available in the Reflex, as I understood. This creates boilerplate code like the one at https://sockpuppet.argpar.se/quickstart-django:

    <a href="#"
    data-reflex="click->CounterReflex#increment"
    data-step="1"
    data-count="{{ count }}"
    >Increment {{ count }}</a>

Why is the data-value needed in the first place?

On the backend side, you need to reference self.element["data-count"] here, scroll down to the 'Untitled' code fragment to get the data. (or self.element.dataset['count'] here? Confusing!

from sockpuppet.reflex import Reflex

class CounterReflex(Reflex):
    def increment(self):
        self.count = (
            int(self.element.dataset['count']) +
            int(self.element.dataset['step'])
        )

It would be much more pythonic to write:

from sockpuppet.reflex import Reflex

class CounterReflex(Reflex):
    # define class variables here, which become automatically 
    # available as template context variables in the frontend
    count = 0
    step = 1

    def increment(self):
        self.count += self.step

At the template, this would be

    <a href="#"
    data-reflex="click->CounterReflex#increment"
    >Increment {{ count }}</a>

and with available parameters:

    <a href="#"
    data-reflex="click->CounterReflex#increment(1)"
    >Increment {{ count }}</a>

    def increment(self, step):

or even dynamically:

    <a href="#"
    data-reflex="click->CounterReflex#increment({{ step }})"
    >Increment {{ count }}</a>

class CounterReflex(Reflex):
    count = 0
    step = 1

    def increment(self, inc):
        # the "inc" param here in this case gets filled with the {{step}} variable again. Just to show a use case.
        self.count += inc
        if inc == 1:
            self.step = 2
        else:
            self.step=1

E.g. Unicorn does it linke this, and it is MUCH simpler, and let's the user set instance variables of the class (here Reflex) which automatically become context variables in the reflex' template.

If I would design an API, I'd do it like this...

So @dylanjw, if I understood correctly, this (instance variables are context variables?) is already the current behavior in SP and you would like to change it? I think the main purpose of designing an API is keeping it simple to understand. Everything else makes it prone to errors on the long term...

[nerdoc]

The data-count={{count}} attr ATM is needed to get the value back to the server. Unicorn uses the unicorn:model attribute for that, much like Vue.js does. But even if you keep the data-<...> attributes for explicit copying values to the backend, on the Reflex side, it would be easier to use instance/class (?) attributes to deal with the values, instead of self.element.dataset*stuff

[nerdoc]

Extending the reflex class with new methods/attributes has the potential to be a breaking change for someone with conflicting variable names in their project. While moving over to using context for passing variables is a bit of a pain now, it sets up the reflex class to be more easily extensible in the future.

Ah ok, now I understand better. Using context is for sure a way to go to not break existing context variables. But again: Unicorn solves this in a brilliant way IMHO: it encapsulates the "component" more than SP: you have a UnicornView which is basically a Reflex, and each of these views has it's own template which renders the "reflex". Context variables are only available within this template. You can then render these components using a templatetag {% unicorn 'my-component' %} This solves the namespace problem and context var clashing IMHO better.

[dylanjw]

Im just concerned with the API for passing context variables from the Reflex class to the client. Getting data back from the client to the Reflex is a bit awkward, but is out of scope for the changes being proposed.

This might really just come down to personal opinion on API design. It feels like the Reflex is mixing concerns by putting context variables to be sent to the client as instance attributes. The Reflex is acting as both an object representing a UI interaction and a special datatype for the template context. It feels weird as a user to make sure my instance methods don't clash with template context variables, and also have to work around having any instance variables I add not just internal to the working of the class/instance but also potentially passed to the client. Putting the context variables under context would feel cleaner to me.

[jonathan-s]

@dylanjw Having slept on this I think the api that you describe makes sense and the arguments you make are sound. Ie

class MyReflex(Reflex):
    def update(self):
        self.context.field = "data"

It should be backwards compatible, so that we can remove the old behaviour in a major version. We should also leave a logging warning if the new api isn't used so that the developer is notified in the command prompt.

[nerdoc]

Ok - I see this is clearly the better solution, and @dylanjw you're right it's a point of view - how API designing is preferred. In this case, as Reflexes always are in context of a whole view, I definitely opt for separation of context as well... :heart:

[nerdoc]

I've slept a few nights on this topic again, and something else occurred to me.

self.context.field = "data"

is great for the writing side of data access. But what about reading? I think this is very confusing as well. If you just want to read an input element's value, you have to write:

name = self.element["attribute"]["value"] 

BTW: according to the docs, the element attribute should have these attributes directly ("The element property contains all of the Reflex controller's DOM element attributes as well as other properties like, tagName, checked and value."), which it doesn't: self.element contains 'attributes, which contains a valuedict key. No properties in [Element](https://github.com/jonathan-s/django-sockpuppet/blob/master/sockpuppet/element.py) exceptdataset`, and the attributes dict.

{
  'reflex': 'change->MyReflex#update', 
  'controller': 'stimulus-reflex', 
  'action': 'change->stimulus-reflex#__perform'
}

which is totally confusing IMHO, and badly documented.

In 99% of all cases I suppose that the user wants to access the "value" of the input field. the element accessor is some sort of low level api (with access to all html attributes etc).

So my suggestion here is something else: Why can't SP provide a higher level API for data reading access?

SP could bind e.g. the value to a value property:

class MyReflex(Reflex):
    def increment(self, step: int = 1):
        name = self.element.value  # shortcut for self.element["attributes"]["value"]
        self.context.upper_name = name.upper()

This would be an approach to get data more easily out of the elements. It's much more readable and pythonic. Please tell me what you think of that.

I could be something like this in the Element's code:

    @property
    def value(self):
        return self.attributes["value"]

    @property
    def checked(self):
        return self.attributes["checked"]

[jonathan-s]

This would be an approach to get data more easily out of the elements. It's much more readable and pythonic. Please tell me what you think of that.

That would totally work for me. However it's worth keeping in mind that you can add any attribute to html so doing it through properties as above is probably not the best solution.

[nerdoc]

... that you can add any attribute to html so doing it through properties as above is probably not the best solution.

That's true. I didn't want to suggest replacing the low-level API with properties. It's just that e.g. value, checked, selectedetc are attrs that are used very often and make sense to have easy accessors. Would make sense to get sane defaults instead of Exceptions too, like when `element.attribute["value"] could raise an Exception when there is no value? The property could just return a "".

[nerdoc]

Ah, and it's not necessary to do hardcoded properties. Could be a __getattr__ generic one.

[jonathan-s]

I think it would also be preferable to keep the api the same as javascript ie for an <a> element with a href attribute it would be the following, and it should stay backwards compatible for the time being.

element.attributes.href

That way it would be consistent. I would also say it would be good to keep this in two separate PRs for the person who takes this on. Just checking in @dylanjw, were you keen on creating a PR for the context.

[nerdoc]

ok - but then there's the docs - ATM they are wrong IMHO, even for the current behaviour. as said, "The element property contains all of the Reflex controller's DOM element attributes as well as other properties like, tagName, checked and value.",

This should (ATM) be: "The element property contains an attribute dict which contains all of the Reflex controllers DOM element attributes as well as other properties like tag_name, checked and value." Mind that "tagName" is completely wrong.

Would be a very small, but important docs improvement, which costs much time for beginners (like me).

[jonathan-s]

@dylanjw FYI, I committed a documentation update here: https://github.com/jonathan-s/django-sockpuppet/commit/76144ed5e8683b5e95458cbfa57b24d05cf4c835 how it works to set a context right now. This will then be updated in #136.