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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lifecycle refactor: introduce new finalize stage, global reflexes dictionary #317

Merged
merged 9 commits into from
Oct 6, 2020

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Sep 25, 2020

Type of PR (feature, enhancement, bug fix, etc.)

Refactoring

Description

What originally started as an exploration of wireframe feedback mechanisms became an overhaul of both when we process post-Reflex lifecycle moments as well as an opportunity to revisit the logging mechanism itself.

Upon investigation, I realized that the library was processing lifecycle mechanisms in response to the before events emitted by CableReady. It would also only report the information for one morph operation, even if there are several. Worse, the durations reported were not accurate as they do not include time spent modifying the DOM. Large, complex DOMs can take several ms to update. Finally, while v3.3.0 improved the callbacks handling significantly, so long as we're executing after callbacks before the operations have actually been called means that there's a very real possibility after callbacks will be based on outdated state.

Now, the tradeoff in moving to using CableReady after events is that there's a chance someone could remove the selector that is supposed to hold their Stimulus controller, meaning that afterReflex callbacks could not fire. I would argue that not only is this a brittle configuration, based on a perception of the library before multiple selector morphs was possible, but that there could easily be a race condition where a DOM element gets removed before or after the current callback is processing. I feel strongly that we should simply explain the situation via documentation: if you remove an element that holds a controller, that controller cannot emit Reflex callbacks.

In terms of the logging itself, I put a lot of energy into thinking about what information is most useful when, and in what format and configuration. We had a lot of long keys, superfluous data and repetition between the message sending to the server and any replies coming back.

Here is the debug output for the following:

    morph "#results", html
    morph "#test1", "<div id='test1'><span>I am content<span></div>"
    morph "#test2", "<div>My content rocks</div>"

selector morphs

Note that the logs are numbered in the order that they finish. This reflects the fact that a morph takes longer to execute. Also, #test2 is not a valid payload for morph so it is processed with inner_html.

  morph :nothing

nothing morph

Nothing morph updates ♾️

  before_reflex do
    throw :abort
  end

throw :abort

So does a halted Reflex.

Why should this be added

This could assist with troubleshooting and debugging, as well as provide insight into what method is being used to update your DOM

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@hopsoft
Copy link
Contributor

hopsoft commented Sep 25, 2020

I like this idea, but think we should iterate on the styling a bit. It might be enough to simply add a CSS class and allow people to define their own styles.

@leastbad
Copy link
Contributor Author

leastbad commented Sep 25, 2020

I'm not attached to anything. The reason I went with an animated approach is because it seems pretty important for the thing to fade out, like a notification.

A more confident JS developer would pass in a closure to the initializer instead of gsap itself. Then we could provide suggested implementations that include versions which just change CSS, one that runs on InertiaJS...

That said, and perhaps off-topic, I believe two things which are not directly related but combine for an outcome: Inertia isn't dead but it's not what it once was; their development is behind, the API is confusing and the docs are more confusing than helpful. And Greensock, which was once a conversion funnel for their paid product, is the incredibly useful, well-designed, well-organized and impressively documented replacement. It's less of an animation tool than an immediate-mode timeline primative that just happens to support scriptable tweening. It is incredible.

Long story short: I am pretty excited to champion a symbiosis between Stimulus and Greensock communities, which I'm confident is something we could spiritually lead on (using it on Expo etc). Almost everywhere people currently use setTimeout would be better served by gsap.timeline, which wraps requestAnimationFrame() with the same elegance that Stimulus wraps MutationObserver.

I use it all of the time just to execute functions on a timeline, with no specific visual outcome. People on the whole haven't realized that you don't need to use it just on mutating geometry or whatever.

@leastbad
Copy link
Contributor Author

Although I realize now that a CSS class could have transitions in it that could make a more sophisticated animation total overkill...

@leastbad leastbad changed the title show wireframes after Reflex operations lifecycle refactor: process callbacks + logs after DOM operations Sep 28, 2020
@leastbad leastbad changed the title lifecycle refactor: process callbacks + logs after DOM operations lifecycle refactor: introduce new finalize stage, global reflexes dictionary Sep 29, 2020
@leastbad
Copy link
Contributor Author

leastbad commented Sep 29, 2020

@hopsoft correctly raised the alarm around what would have been a major version-breaking change. Upon consideration, it became clear that the correct approach was to introduce a new lifecycle stage, finalize which runs after all DOM operations have completed.

Implementing this functionality took me on a Fantasia ride through the codebase, allowing me to do significant Fall cleaning. We had chunks of code which were never called (likely my fault!) and we had quietly accumulated some technical debt in the form of tracking multiple dictionary objects all keyed to the reflexId in addition to the promise itself. Overall many internal method signatures are much simpler; for example, Log.success and Log.error both now accept event and are able to access everything else that they need from the global dictionary.

While the introduction of the finalize stage is the most visible change, the more dramatic and exciting difference is the addition of the new global reflexes dictionary, which replaces promises, Log.logs and ends the practice of attaching data to the promise (with the exception of reflexId which is user-accessible):

reflexes[reflexId] = {
  finalStage: ['after', 'finalize', 'halted'],
  totalOperations,
  pendingOperations,
  completedOperations,
  promise,
  timestamp
}

What this means is that everywhere in the client library you can access the dictionary and with a reflexId you can then access the promise or any of the other metadata used to make the sausage. finalStage defaults to 'finalize' and controls when reflex entries are removed from the global dictionary. There is no longer any need to clean up promises or logs individually.

A substantial amount of energy in this PR went into finally getting all of the promises, events and callbacks to fire reliably in the correct order. After much trial and error, the correct technique was to move all setTimeout functions out of lifecycle.js. Everything now works like a dream; I've tested with: page, selector, nothing, halted, and an error with events, callbacks and the promise in both early and late resolution modes. (More on that in a sec.)

Making callbacks and events work with the new finalize event was easy. Promises is much harder since you can only resolve/reject once. There is now a resolveLate option which can be passed to stimulate which defaults to false. The default dehaviour is also the current behaviour. However, if you pass resolveLate: true it will hold off on resolving the promise for this reflexId until after all of the DOM updates are completed.

I know that a PR like this can be annoying to test as there's lots of small changes. If folks are interested, I'd be happy to whip up a harness branch that tests all of the functionality here.

Here's three selector morphs with console logs for all of the lifecycle events with resolveLate: false (default):
chrome_63JeohwWZu

Now, here's the same reflex except with resolveLate: true:
chrome_8hmwg6FZpm

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.

Great job on this PR! I only had one minor pedantic question. 😉

javascript/stimulus_reflex.js Outdated Show resolved Hide resolved
@julianrubisch
Copy link
Contributor

I can give this a 👍 , not seeing any timing-bugs. Could we cut a pre-release with that soon?

@hopsoft hopsoft merged commit a3d289a into stimulusreflex:master Oct 6, 2020
@leastbad leastbad mentioned this pull request Oct 21, 2020
2 tasks
@leastbad leastbad deleted the wireframe branch August 16, 2021 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants