Consolidate event handling into Listener type. #245

Laimiux · 2021-09-30T21:45:54Z

What

Added Listener type (maybe should call it Callback instead). This replaces () -> Unit and (Event) -> Unit
Renamed TransitionCallbackWrapper to TransitionDispatcher.

Laimiux · 2021-09-30T21:51:26Z

formula/src/main/java/com/instacart/formula/BoundStream.kt

@@ -0,0 +1,41 @@
+package com.instacart.formula
+
+class BoundStream<Message>(


Renamed from Update to BoundStream

Moving this into #246

Laimiux · 2021-09-30T22:08:09Z

formula/src/main/java/com/instacart/formula/Listener.kt

+/**
+ * Formula uses Listener callbacks to pass events.
+ */
+fun interface Listener<Event> : (Event) -> Unit {


Still debating what would be the right name for this.

I prefer Listener

Another option would be Handler. I don't have a strong preference, just throwing it out there to see what other people think.

Yeah, I've also thought about Handler though I didn't like its associations with android.Handler

Laimiux · 2021-09-30T22:08:24Z

formula/src/main/java/com/instacart/formula/TransitionContext.kt

@@ -0,0 +1,30 @@
+package com.instacart.formula
+
+interface TransitionContext {


Not yet sure about moving this out.

Laimiux · 2021-09-30T22:12:14Z

formula/src/main/java/com/instacart/formula/Update.kt

+ * and/or [Effects] that should be performed. You can return [Transition.None] if nothing
+ * should happen.
+ */
+fun interface Update<State, Event> {


I'm wondering how to better unify this with the Transition type. Maybe this should be changed to

fun interface Transition<State, Event> { sealed class Result { /** * Stateful transition. * * @param state New state * @param effects Optional effects such as parent callbacks, logging, db writes, * network requests, etc. */ data class Stateful<State>(val state: State, override val effects: Effects? = null) : Result<State>() /** * Only effects are emitted as part of this transition * * @param effects Effects such as parent callbacks, logging, db writes, network requests, etc. */ data class OnlyEffects(override val effects: Effects) : Result<Nothing>() /** * Nothing happens in this transition. */ object None : Result<Nothing>() { override val effects: Effects? = null } } fun TransitionContext.create(event: Event): Result<State> }

Yeah, having an Update that produces a Transition is a bit confusing, would be better to not have that extra name I think.

Does Update and Update.Result or Transition and Transition.Result seem better? On one hand, Update corresponds to MVU naming, however many folks like the name Transition (including me).

I'm moving this change out of this PR.

Laimiux · 2021-09-30T22:14:42Z

formula/src/main/java/com/instacart/formula/internal/TransitionDispatcher.kt

+import com.instacart.formula.Update
+
+@PublishedApi
+internal class TransitionDispatcher<State>(


This was called TransitionCallbackWrapper previously

Laimiux · 2021-09-30T22:15:06Z

formula/src/main/java/com/instacart/formula/internal/TransitionDispatcher.kt

+    }
+
+    // TODO: naming UpdateType as transition seems weird. We need to resolve these two types.
+    fun <Event> dispatch(transition: Update<State, Event>, event: Event) {


TODO: fix this once we resolve naming for Update

Laimiux · 2021-09-30T22:31:01Z

formula/src/main/java/com/instacart/formula/internal/FormulaManagerImpl.kt

        children?.forEachValue { it.markAsTerminated() }
    }

    override fun performTerminationSideEffects() {
        children?.forEachValue { it.performTerminationSideEffects() }
        updateManager.terminate()
+        callbacks.disableAll()


The reason I'm moving this from markAsTerminated to performTerminationSideEffects is to allow termination side-effects to execute.

This was needed for https://github.com/instacart/formula/pull/245/files#r719839477

carrotkite · 2021-09-30T23:09:47Z

JaCoCo Code Coverage 79.03% ✅

Class	Covered	Meta	Status
`com/instacart/formula/internal/Listeners`	80%	0%	✅
`com/instacart/formula/internal/FormulaManagerImpl`	88%	0%	✅
`com/instacart/formula/internal/Frame`	92%	0%	✅
`com/instacart/formula/internal/FormulaContextImpl`	50%	0%	✅
`com/instacart/formula/internal/ListenerImpl`	75%	0%	✅
`com/instacart/formula/Evaluation`	100%	0%	✅
`com/instacart/formula/FormulaContext`	47%	0%	✅
`com/instacart/formula/StatelessFormula`	100%	0%	✅
`com/instacart/formula/test/TestListener`	0%	0%	✅

Generated by 🚫 Danger

Laimiux · 2021-09-30T23:21:10Z

formula/src/main/java/com/instacart/formula/internal/CallbackImpl.kt

+ * Note: this class is not a data class because equality is based on instance and not [key].
+ */
+@PublishedApi
+internal class CallbackImpl<State, Event>(internal var key: Any) : Listener<Event> {


TODO: this should be renamed to ListenerImpl or Listener should be renamed to Callback

Maybe add this as a comment to this class?

Most likely, I'm going to rename this before merging the PR.

Laimiux · 2021-09-30T23:26:18Z

formula/src/main/java/com/instacart/formula/FormulaContext.kt

-            }
-
+            val key = JoinedKey(stream.key(), transition::class)
+            val callback = formulaContext.onEvent(key, transition)


This now delegates to onEvent to create the Stream listener/callback. This was made possible, by https://github.com/instacart/formula/pull/245/files#r719810293

alexanderbezverhni

LGTM 👍🏼

alexanderbezverhni · 2021-10-01T15:32:14Z

formula/src/main/java/com/instacart/formula/FormulaContext.kt

     */
-    inline fun <UIEvent> onEvent(


Any reason we migrated off inline for all functions here?

Previously, we were creating another function inside of here and cross-inlining the transition function to save an extra object allocation. I've restructured this, so inline isn't needed anymore.

alexanderbezverhni · 2021-10-01T15:35:31Z

formula/src/main/java/com/instacart/formula/Listener.kt

+}
+
+/**
+ * Utility function used to specific


INcomplete comment?

alexanderbezverhni · 2021-10-01T15:40:37Z

formula/src/main/java/com/instacart/formula/internal/CallbackImpl.kt

+ * Note: this class is not a data class because equality is based on instance and not [key].
+ */
+@PublishedApi
+internal class CallbackImpl<State, Event>(internal var key: Any) : Listener<Event> {


Maybe add this as a comment to this class?

alexanderbezverhni · 2021-10-01T15:43:54Z

formula/src/test/java/com/instacart/formula/subjects/EventCallbackFormula.kt

@@ -17,7 +17,7 @@ class EventCallbackFormula : Formula<Unit, String, EventCallbackFormula.Output>
        return Evaluation(
            output = Output(
                state = state,
-                changeState = context.onEvent { newState ->
+                changeState = context.onEvent<String> { newState ->


This actually adds up to the code readability.

Do you mean improves code readability?

johnwilde · 2021-10-01T15:59:31Z

formula/src/main/java/com/instacart/formula/Update.kt

+ * and/or [Effects] that should be performed. You can return [Transition.None] if nothing
+ * should happen.
+ */
+fun interface Update<State, Event> {


Yeah, having an Update that produces a Transition is a bit confusing, would be better to not have that extra name I think.

mattkranzler5

Looks good, I like the changes.

Laimiux · 2021-10-01T19:13:47Z

Hey folks, thank you for your initial review 💯

To reduce the scope of this PR, I've moved some changes such as TransitionContext and Update out (I want to fully flesh these out before merging). In this PR, I focused on the Listener change and update all the docs. Please take a quick look again to see if anything doesn't make sense cc @johnwilde @mattkranzler5 @alexanderbezverhni

jasonostrander

LGTM, just one question.

jasonostrander · 2021-10-01T19:33:16Z

formula/src/test/java/com/instacart/formula/subjects/KeyUsingListFormula.kt

@@ -17,7 +18,7 @@ class KeyUsingListFormula :

    data class ItemRenderModel(
        val item: String,
-        val onDeleteSelected: () -> Unit
+        val onDeleteSelected: Listener<Unit>


Does this change mean we need to type our callbacks as Listener in RenderModels, or is this change specific to these formulas?

When using onEvent you will need to type to Listener type (or (Event) -> Unit). For now, there still exists callback and eventCallback which support old style.

Laimiux commented Sep 30, 2021

View reviewed changes

Laimiux force-pushed the laimonas/naming branch 2 times, most recently from 5c4aee3 to 943b092 Compare September 30, 2021 21:55

Laimiux requested review from johnwilde, mattkranzler5, Jawnnypoo, FrancoisBlavoet and alexanderbezverhni September 30, 2021 22:07

Laimiux commented Sep 30, 2021

View reviewed changes

Laimiux force-pushed the laimonas/naming branch 2 times, most recently from a159563 to c39f88b Compare September 30, 2021 23:00

Laimiux added 6 commits September 30, 2021 16:16

Rename TransitionCallbackWrapper to TransitionDispatcher.

55ca139

Adding UpdateType (not finalized name).

a4b6e2d

Adding Listener type (name is still TBD).

3958f1d

Remove extra lambda allocation.

90b2d8d

Rename Update to BoundStream.

6b14d84

Reuse formulaContext.onEvent for Stream event handling.

0220dd6

Laimiux force-pushed the laimonas/naming branch from c39f88b to 3b38fad Compare September 30, 2021 23:20

Laimiux commented Sep 30, 2021

View reviewed changes

Fix samples.

0bc9a30

Laimiux force-pushed the laimonas/naming branch from 3b38fad to 0bc9a30 Compare September 30, 2021 23:25

Laimiux commented Sep 30, 2021

View reviewed changes

alexanderbezverhni approved these changes Oct 1, 2021

View reviewed changes

johnwilde approved these changes Oct 1, 2021

View reviewed changes

mattkranzler5 approved these changes Oct 1, 2021

View reviewed changes

Laimiux added 4 commits October 1, 2021 10:51

Reset Update.kt change.

b753a30

Extend Listener documentation.

6150bd5

Update all docs.

842ab65

Add TestListener.

28dda6d

Laimiux changed the title ~~Consolidate event handling and naming.~~ Consolidate event handling into Listener type. Oct 1, 2021

jasonostrander approved these changes Oct 1, 2021

View reviewed changes

Jawnnypoo approved these changes Oct 1, 2021

View reviewed changes

Laimiux merged commit d726694 into master Oct 1, 2021

Laimiux deleted the laimonas/naming branch October 1, 2021 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate event handling into Listener type. #245

Consolidate event handling into Listener type. #245

Laimiux commented Sep 30, 2021 •

edited

Loading

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

johnwilde Oct 1, 2021

mattkranzler5 Oct 1, 2021

Laimiux Oct 1, 2021

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

johnwilde Oct 1, 2021

Laimiux Oct 1, 2021

Laimiux Oct 1, 2021

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

Laimiux Sep 30, 2021

carrotkite commented Sep 30, 2021 •

edited

Loading

Laimiux Sep 30, 2021

alexanderbezverhni Oct 1, 2021

Laimiux Oct 1, 2021

Laimiux Sep 30, 2021 •

edited

Loading

alexanderbezverhni left a comment

alexanderbezverhni Oct 1, 2021

Laimiux Oct 1, 2021

alexanderbezverhni Oct 1, 2021

Laimiux Oct 1, 2021

alexanderbezverhni Oct 1, 2021

alexanderbezverhni Oct 1, 2021

Laimiux Oct 1, 2021

johnwilde Oct 1, 2021

mattkranzler5 left a comment

Laimiux commented Oct 1, 2021

jasonostrander left a comment

jasonostrander Oct 1, 2021

Laimiux Oct 1, 2021 •

edited

Loading

		@@ -0,0 +1,41 @@
		package com.instacart.formula

		class BoundStream<Message>(

		@@ -0,0 +1,30 @@
		package com.instacart.formula

		interface TransitionContext {

Consolidate event handling into Listener type. #245

Consolidate event handling into Listener type. #245

Conversation

Laimiux commented Sep 30, 2021 • edited Loading

What

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carrotkite commented Sep 30, 2021 • edited Loading

JaCoCo Code Coverage 79.03% ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laimiux Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

alexanderbezverhni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkranzler5 left a comment

Choose a reason for hiding this comment

Laimiux commented Oct 1, 2021

jasonostrander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laimiux Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

Laimiux commented Sep 30, 2021 •

edited

Loading

carrotkite commented Sep 30, 2021 •

edited

Loading

Laimiux Sep 30, 2021 •

edited

Loading

Laimiux Oct 1, 2021 •

edited

Loading