-
-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor SR client package and introduce the Reflex class #592
Refactor SR client package and introduce the Reflex class #592
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice refactor here. Also the devex improvements that the store of reflexes provides is fantastic.
I'm well aware of the conflicting files. I don't want to merge/rebase until the issue with #597 is properly resolved. |
I had to pull out the morph broadcaster changes that came with #597, @julianrubisch. The intent of the PR was reasonable, but the impact in this case had further reaching issues; we didn't consider that other parts of the library (such as the custom logger) assume that all Reflexes have a viable Also, once we added stubs for halted, error and forbidden Reflexes, we ended up with a substantial amount of not-DRY code that actually made things more brittle. We can revisit this, but for now, I had to make a call. |
Type of PR
Refactor
Description
This PR refactors the SR client package to use the new
Reflex
object - living in the globalreflexes
store - while still maintaining legacy API support for the final v3.x release. It represents a massive simplication of the internal workings of the library and will make the release of v4 relatively straight-forward.Historically, SR has made extensive use of object variables attached to DOM objects to maintain state. Over time and as the sophistication of the library has grown, more variables have been introduced. Given that these variables are themselves dictionaries of multiple Reflexes at different stages of their life cycle - and factoring in support for "non-isolation mode" complicating everything - this has slowly boiled up a pot of serious technical debt, leading to a general perception that this library is "hard", "complicated" and that there's roughly 1.0 humans who fully understand how it works. This is not a viable scenario, and it guarantees that would-be potential contributors are rightfully scared off.
The solution is to formally create a Reflex class, and standardize every interface in the framework to operate on instances of this class. This immediately leads to deleting huge amounts of code, and v4 developers will find that they are always working with a consistent API, regardless of how they approach their application architecture.
There will no longer be any need to manually navigate data structures attached to DOM objects to do advanced things. There will be no crazy method signatures in callbacks. I'm pretty sure that we will be able to completely remove the
xpath_controller
andxpath_element
stuff in v4, which makes the payload smaller and further reduces the complexity of the library.Essentially, this PR allows 3.5 to run 4.0 code while presenting the 3.4.x API surface.
The existence of a Reflex class now allows developers to reason about Reflex requests programmatically, instead of a collection of concepts that they have to trust to produce a magical result. Reflexes have a
stage
accessor that is updated when the stage changes. [I have resisted the temptation to add state machine transition callbacks and subclassable Reflex classes... for now. 😉]It is easy to test Reflex class instances. The class itself has member properties for everything the developer might need;
data
is the payload which was sent to the server, andpromise
is a pointer to the Promise.The
StimulusReflex.initialize
method now returns a reference toreflexes
which I have taken to assigning towindow.reflexes
in development mode.reflexes
is a Proxy to an internal object that allows keyed access to the Reflexes stored inside, as you currently do in pre9. However, it now also offers scoping accessors for each possible state.Similarly, Stimulus controllers that have been
register
ed as SR controllers now expose athis.reflexes
property which is scoped to return only the Reflexes created by this instance of the Stimulus controller. Developers will be able to usethis.reflexes.finalized
andthis.reflexes.last
in their controllers; yes,this.reflexes.last
returns the last Reflex created by that instance of the controller. 🙇The source is extensively marked up with comments suggesting code that can be removed or easily changed for v4. This should further ease the transition and turnaround time once 3.5 is out.
Support for multiple transports has been enabled on the client. There is no AC-specific code in
stimulus_reflex.js
that cannot be swapped out, @jonathan-s. Theinitialize
method now accepts atransport
key, which defaults toActionCableTransport
. TheActionCableTransport
module exports only three methods and can be used as a starting point for implementing other transports. Note that there's currently no Ruby-side support for non-Action Cable transports.As part of the refactoring, I extracted as much logic from
stimulate
as possible; into util functions, into classes and modules. The Stimulus Application is now a module, and there is basically a bare minimum of distraction from the method's core responsibility, which is to prepare a new Reflex for delivery to the server. I believe that the simpler we can makestimulate
, the easier it will be for new contributors to learn the internals. I also got rid of most of the duplication inlog.js
that was making @julianrubisch sad.There's 2.5 new lifecycle stages: Reflex instances are briefly
created
before switching tobefore
. When they are sent to the transport for delivery, they become eitherdelivered
orqueued
(and hopefully soondelivered
).Reflex instances now have a
lifecycle
accessor, which returns an array containing every life-cycle event that has triggered. For example, you can inspect the events for the last Reflex; in this case, a Nothing Morph:Note that very few changes have been made to the Ruby library to support this. The
broadcast_error
method now transmits anerror
instead of abody
. A missing variable that will cause an error in pre9 today has been renamed.Why should this be added
While it might seem like this is out of nowhere, I have been working on the ideas behind this refactor for over a year. Many initiatives emerged from conversations with @hopsoft in 2020, where I described creating a Reflex object and having
stimulate
be the way developers create those Reflexes, like a factory.As the horizon for a 3.5 continues to stretch and bend, my attitude towards the changes in this PR is part impatience, part optimism and part existential despair, both personally and for the library. Indeed, the clock is ticking on our ability to stay relevant and maintain mindshare... but I also worry about my health, and don't want to sit on work that might otherwise never see light should something ever happen to me. 🛸
Here's the thing: it's taken a long time to get 3.5 out the door, for lots of good reasons that don't matter to people who aren't us. What I hope with this PR is that we can actually follow up 3.5 with a 4.0 release "relatively quickly" if we make 3.5 through the lens of the library we want instead of the technical debt that we're stuck with.
I believe that the technical debt which accrued in the 3.4.x cycle is significant enough that it effectively makes building plugin features for SR cynical and impossible. Trying to juggle all of the hanging variables without any real OOP model while not having firm guarantees about internal API is enough to piss off even the most good natured contributors. (Hi, @joshleblanc! 💟 Thank you for all that you do.)
This PR does the work to clean up the debt and expose an API we can be truly proud of, allowing for a bright and extensible future for this library.
Checklist