refactor views and add ComponentResponse class to help handle requests

[jacksund]

Summary

The end goal is to make code easier to read/follow for new devs, without changing anything about how users create apps. Changes include:

• refactor views/.__init__.py into views/message.py. So message fxn is now a class: UnicornMessageHandler
• added ComponentResponse class to help distinguish from ComponentRequest
• added helper methods to ComponentRequest to simplify higher-level use in request handling
• changed Action to be an abstract base class, rather than a generic. And added subclasses for each action type (CallMethod, Reset, Refresh, Toggle, SyncInput, etc.)

Motivation

I was trying to help find a fix for several issues (#650 #662 #666), but was struggling to follow along with some of the code. It is heavily focused on large functions and modifying objects in-place, rather than more well defined classes + methods. I'm sure others have ran into this issue before, so I started a refactor in hopes to make the code cleaner & easier to follow.

Details of changes

Articulating changes in a refactor like this is difficult, but here's my stab at describing what I changed & why:

• ComponentRequest is not treated as immutiable, and it is gradually converted to a "ComponentResponse" within various methods. I decoupled these steps so that's it's easier to follow, and I also added a ComponentResponse base class to help some more
• The call_method.handle overcomplicates a lot of data passing -- especially in how it generates data in _process_component_request and generates Return objects. Lots of this can be simplified by further developing the Action and ComponentResponse classes, so I added several methods/properties to these classes to help.
• UnicornView is modified in various locations, but it's much easier to follow when there are reusable methods on the class directly. I "pushed down" some code from views.__init__.py into the UnicornView to make higher-level functions easier to follow + create more reuseable UnicornView methods.

[jacksund]

This is a work in progress, and will certainly need feedback & discussion if it's ever to be accepted. I realize changes this large have a chance of being rejected, so I decided to open the PR while it's still in a draft state.

@adamghill I'll continue to work on the refactor for now, but let me know if this heads in a direction you don't approve of 😄

[jacksund]

looks like black is adding changes where I haven't edited anything. Is this not the black version you normally use?

django-unicorn/poetry.lock

Line 149 in 07babfb

version = "23.12.1"

[adamghill]

I really appreciate you digging into this! This code is... extremely gnarly. It got built up over lots of time. Looking forward to seeing how this progresses. I have decent test coverage last I checked (something like 90%?) however, this will need extensive manual testing as well because of where it lives.

I've slowly been moving from black to ruff for auto-formatting. Maybe that's why?

[jacksund]

I really appreciate you digging into this! This code is... extremely gnarly. It got built up over lots of time. Looking forward to seeing how this progresses.

Awesome! I'm glad you're open to it so I'll keep working away 🥳. I think it's going to take me the weekend to get this MR to a state where I'm ready for you to give a peek / feedback. I'll ping you when I hit that stage

I have decent test coverage last I checked (something like 90%?) however, this will need extensive manual testing as well because of where it lives.

Haha yeah, I haven't touched the test suite yet, but have been doing manual checks along the way. If it get's too repetitive, I might write a test using the selenium package

I've slowly been moving from black to ruff for auto-formatting. Maybe that's why?

Ah, gotcha. I haven't used ruff before but I'll switch over to that

[adamghill]

might write a test using the selenium package

Have you looked at playwright at all? I did a proof of concept at some point for unicorn although I don't think I ever pushed it. It might be the way to go for end-to-end testing, although I haven't done any selenium tests in a very long time so I'm not sure!

[jacksund]

Have you looked at playwright at all?

I haven't! I'll check that one out too

[jacksund]

I'm going to pause work on this PR for a couple days. It is still a work in progress and not ready for any manual or automated testing because I need to finish up some subclass definitions (such a actions.frontend.Redirect) and run manual checks myself before anyone tries

But I would say this PR is ready for feedback at the concept level:

• what do you think of the new action module? and it's submodules for frontend and backend?
• does the reorganization of the views module make sense?
• does the new class organization make sense? specifically... Component (aka UnicornView), ComponentRequest, ComponentResponse, BackendAction, and FrontendAction

To help with skimming through:

• start by doing a big-picture skim of actions module (only look at class names really). You'll see views.action_parsers is now refactored into actions.backend. Likewise, Return and Update objects were merged and moved to actions.frontend
• when reading code, start with the UnicornMessage.post method within views.message module. This is the new entry point and everything stems from there.

[jacksund]

just to add -- sorry this is such a massive PR. I know that can cause headache as a maintainer 😅. So I wouldn't spend any more than 10 min looking through for now. I'll let you know when its actually ready for a full code review though

[jacksund]

manually checking different types of unicorn setups and actions:

• basic CallMethod
• basic type args to CallMethod
• coerced type args to CallMethod
• django model type arg to CallMethod
• custom type args to CallMethod
• enum type args to CallMethod
  • ⚠️ enum works on its own but breaks in combination with other CallMethod. This bug also exists in main so it is not associated with this PR
• SetProperty shortcut
• $returnValue special arg
  • ⚠️ I need a better example in the docs. As-is, I don't have this working using either main or this PR -- probably because I'm using it wrong
• $parent special arg
• $refresh special method
  • ⚠️when there's nothing to refresh, this just returns RenderNotModifiedError. That's expected behavior, right?
• $reset special method
• $toggle special method
• $validate special method
• Unicorn.call for CallMethod
• Unicorn.getReturnValue
• filter example in parent-child component
• multiple child example in parent-child component
• Redirect
• HashUpdate
• LocationUpdate
• Partial update
• PollUpdate

[jacksund]

This MR is ready for a review -- but not merge.

Remaining to-do items before merging:

• allow SERIAL.ENABLED = True: I have it raise NotImplementedError for now, and might need help with this
• update unittests: I'm waiting on feedback before I take on changing the tests

@adamghill no rush on this 😄 . I can only help out on the weekends atm, so I might not get to your feedback right away anyways

[jacksund]

as a side -- do you squash commits on merge? I'm pretty loose with how often I commit and with what the commit messages are

[adamghill]

as a side -- do you squash commits on merge

I usually do! Although probably easiest if you rebase when you get to a good place and force push to your branch. Then I can just do a clean merge.

[jacksund]

I'm going to revisit this PR if that's okay. There are a lot of issues being opened about child components that I'd like to help with, but the current state of the source code makes debugging pretty difficult -- in fact, I could only track down the issue for #666 within this PR, rather than on the main branch

[adamghill]

I'm going to revisit this PR if that's okay.

That would be great! I look forward to seeing how if this code could be easier to grok. :)

[jacksund]

I kinda went overboard on the refactor. There are the many changes that I summarized above, and then in the 2 most recent commits:

• I rewrote a lot of the UnicornView class and its methods. The class was renamed to Component but still has the UnicornView alias available/used throughout the package
• To help with bug fixes, I needed to disable both queueing and django-based caching. This means components will only work on a single instance of django -- so a load balancer + redis caching aren't utilized. (I plan on re-adding this though)
• in order to fix updating child components via a parent method #666, I had to add this hacky chunk of code here. This was done because I don't know how to update the JS on the frontend (which sends back incorrect data)

[jacksund]

I think there are too many changes (and several feature regressions) where it's not reasonable to ask for an MR review + eventual merge. However, this can still serve as an outline/model for future refactors in the package.

Because of my own deadlines (🤢), I need this fork and a couple of its fixes in my prod env -- so I'm likely going to "hard fork" from here and use this code within my own packages, where I'll gradually continue to work on it. Hopefully over time, some of its refactor choices make it in this repo though!

I'm going to close this MR, just to indicate it's not planned for any review or merging