Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor SR client package and introduce the Reflex class #592

Merged
merged 28 commits into from Sep 14, 2022

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Jul 2, 2022

Type of PR

Refactor

Description

This PR refactors the SR client package to use the new Reflex object - living in the global reflexes 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 and xpath_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, and promise is a pointer to the Promise.

The StimulusReflex.initialize method now returns a reference to reflexes which I have taken to assigning to window.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.

reflexes.all // every reflex in the store
reflexes.queued // reflexes that are in the queued state, already used in ActionCableTransport
reflexes.last // the last reflex that was created

Similarly, Stimulus controllers that have been registered as SR controllers now expose a this.reflexes property which is scoped to return only the Reflexes created by this instance of the Stimulus controller. Developers will be able to use this.reflexes.finalized and this.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. The initialize method now accepts a transport key, which defaults to ActionCableTransport. The ActionCableTransport 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 make stimulate, the easier it will be for new contributors to learn the internals. I also got rid of most of the duplication in log.js that was making @julianrubisch sad.

There's 2.5 new lifecycle stages: Reflex instances are briefly created before switching to before. When they are sent to the transport for delivery, they become either delivered or queued (and hopefully soon delivered).

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:

reflexes.last.lifecycle
// ['created', 'before', 'delivered', 'nothing', 'success', 'after']

Note that very few changes have been made to the Ruby library to support this. The broadcast_error method now transmits an error instead of a body. 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

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@leastbad leastbad added enhancement New feature or request proposal javascript Pull requests that update Javascript code labels Jul 2, 2022
@leastbad leastbad added this to the 3.5 milestone Jul 2, 2022
@leastbad leastbad self-assigned this Jul 2, 2022
Copy link
Contributor

@hopsoft hopsoft left a 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.

javascript/reflex.js Outdated Show resolved Hide resolved
javascript/reflex_store.js Outdated Show resolved Hide resolved
javascript/reflexes.js Outdated Show resolved Hide resolved
javascript/stimulus_reflex.js Show resolved Hide resolved
javascript/stimulus_reflex.js Show resolved Hide resolved
javascript/transports/action_cable.js Show resolved Hide resolved
@leastbad
Copy link
Contributor Author

leastbad commented Aug 9, 2022

I'm well aware of the conflicting files. I don't want to merge/rebase until the issue with #597 is properly resolved.

@leastbad
Copy link
Contributor Author

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 broadcaster.

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.

@leastbad leastbad merged commit f166233 into stimulusreflex:master Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants