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

Rename Stream to Action. #229

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Rename Stream to Action. #229

merged 1 commit into from
Mar 24, 2022

Conversation

Laimiux
Copy link
Collaborator

@Laimiux Laimiux commented Sep 10, 2021

I want to rename Stream to Action

  • I think name is more fitting as it represents a deferred action that is executed and managed by Formula runtime.
  • Renaming updates to actions will keep the naming more consistent
  • Action.onInit and similar methods seems better named than Stream.onInit
Old New
Stream Action
RxStream RxAction
FlowStream FlowAction
BoundedStream BoundedAction
Evaluation.updates Evaluation.actions

package com.instacart.formula

/**
* An action returned by [evaluation][Formula.evaluate] that will run for any new unique
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more elaborated documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 on being explicit on what the contract is here in regard to e.g. key

*
* Use this stream to send a effects with latest [Data] value.
* ```
* events(Stream.onData(itemId)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: need to audit this and rename it.

@@ -121,7 +121,7 @@ abstract class FormulaContext<State> internal constructor(
class UpdateBuilder<State>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: I'd like to move this out of into an ActionBuilder

*/
data class Evaluation<out Output>(
val output: Output,
val updates: List<Update<*>> = emptyList()
)
val actions: List<BoundAction<*>> = emptyList()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have better naming downstream, it might be worth wrapping this into a data class?

data class Actions(val list: List<ICBoundAction<*>>)

@@ -99,7 +99,7 @@ abstract class FormulaContext<State> internal constructor(
/**
* Provides an [UpdateBuilder] that enables [Formula] to declare various events and effects.
*/
abstract fun updates(init: UpdateBuilder<State>.() -> Unit): List<Update<*>>
abstract fun updates(init: UpdateBuilder<State>.() -> Unit): List<BoundAction<*>>
Copy link
Collaborator Author

@Laimiux Laimiux Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: add actions method.

@@ -170,7 +170,7 @@ abstract class FormulaContext<State> internal constructor(
this@UpdateBuilder.events(stream, transition)
}

@PublishedApi internal fun add(connection: Update<*>) {
@PublishedApi internal fun add(connection: BoundAction<*>) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: renamed connection to action.

*/
fun key(): Any?
}
typealias Stream<Message> = DisposableAction<Message>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: add @Deprecated

@carrotkite
Copy link

carrotkite commented Sep 10, 2021

JaCoCo Code Coverage 80.64% ✅

Class Covered Meta Status
com/instacart/formula/coroutines/FlowFormula 100% 0%
com/instacart/formula/internal/SnapshotImpl 50% 0%
com/instacart/formula/internal/FormulaManagerImpl 87% 0%
com/instacart/formula/Evaluation 100% 0%
com/instacart/formula/FormulaContext 58% 0%
com/instacart/formula/ActionFormula 100% 0%
com/instacart/formula/StreamFormula 100% 0%
com/instacart/formula/ActionBuilder 100% 0%

Generated by 🚫 Danger

*/
typealias Effects = () -> Unit
typealias Action = () -> Unit
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a good name could be DeferredAction.


/**
* Handles [Update] changes.
* Handles [BoundAction] changes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: rename this class to ActionManager

@Laimiux Laimiux changed the title [Exploration] Renaming to Action and DisposableAction. [Exploration] Renaming to Action and DisposableAction. (WIP) Sep 10, 2021
Copy link
Collaborator

@johnwilde johnwilde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few nits on doc syntax

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch 3 times, most recently from e92b245 to bec80b6 Compare September 13, 2021 22:01
@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch 2 times, most recently from d678ab2 to 1f741ca Compare September 29, 2021 21:54
Copy link
Collaborator

@FrancoisBlavoet FrancoisBlavoet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commenting so it disappears from my github PR list, happy to review it when it is updated

package com.instacart.formula

/**
* An action returned by [evaluation][Formula.evaluate] that will run for any new unique
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 on being explicit on what the contract is here in regard to e.g. key

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 1f741ca to ffd7db3 Compare March 22, 2022 23:44
@Laimiux Laimiux changed the title [Exploration] Renaming to Action and DisposableAction. (WIP) Rename Stream to Action. Mar 22, 2022
@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from ffd7db3 to 1fda990 Compare March 23, 2022 00:02
@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 922dd0b to 8d9b7d6 Compare March 23, 2022 00:15
val fetchUserStream = RxStream.fromObservable { repository.fetchUser() }
fetchUserStream.onEvent { userResult ->
// Do something
val fetchUserAction = RxAction.fromObservable { repository.fetchUser() }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads better to me than fetchUserStream

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 8d9b7d6 to 4fed82d Compare March 23, 2022 00:18
* Adapter which maps RxJava types to [Action] type. Take a look
* at [RxAction.fromObservable].
*/
interface RxAction<Event> : Action<Event> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasted from RxStream with documentation changes.

*
* @param Event A type of event used to notify [Formula].
*/
interface Action<Event> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasted from Stream with big documentation changes

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still see Stream in the docs below. Intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated

* until [action][action] produces a value. It will recreate and resubscribe to
* the [Action] whenever [Input] changes.
*/
abstract class ActionFormula<Input : Any, Output : Any> : IFormula<Input, Output> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy pasted from StreamFormula

/**
* Builds a list of deferred actions that will be executed by Formula runtime.
*/
abstract fun actions(init: ActionBuilder<Input, State>.() -> Unit): List<BoundAction<*>>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be buildActions?

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 4fed82d to 6e0a1f9 Compare March 23, 2022 00:25
Copy link
Member

@Jawnnypoo Jawnnypoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe onEvent could instead be onAction with this name change? Just a thought

@FrancoisBlavoet
Copy link
Collaborator

Maybe onEvent could instead be onAction with this name change? Just a thought

what we get in "onEvent" is the result of the action though, unless I am very confused about the direction of this change, that would still be an event to me (or another "not action" naming),

docs/diffing.md Outdated
@@ -3,5 +3,5 @@ TODO
### Diffing
Given that we recompute everything with each state change, there is an internal diffing mechanism with Formula. This
mechanism ensures that:
1. RxJava streams are only subscribed to once.
1. RxJava actions are only subscribed to once.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still be streams to keep with Rx terminology?

@mattkranzler5
Copy link
Contributor

@Laimiux Just spitballing here but I'm wondering if Event makes more sense than Action. The callbacks which were renamed to onEvent() indicate to me that it's in response to a single Event where as the Stream concept is in response to handling multiple Events. This semantically to me looks more correct:

context.events {
    Events.fromData(...).onEvent {
        ...
    }
    Events.fromObservable(...).onEvent {
        ...
    }
    Events.fromFlow(...).onEvent {
        ...
    }
}

Thoughts?

@Laimiux
Copy link
Collaborator Author

Laimiux commented Mar 23, 2022

Maybe onEvent could instead be onAction with this name change? Just a thought

what we get in "onEvent" is the result of the action though, unless I am very confused about the direction of this change, that would still be an event to me (or another "not action" naming),

That's correct, for example readUserFromDatabase is an action which then emits an event with user state. So, onEvent handles events emitted by the action.

Just spitballing here but I'm wondering if Event makes more sense than Action...

@mattkranzler5 I guess in my mind event is something that an action can produce/emit. For example, you can have an action which saves data to the backend and emits an event of success/failure status.

RxAction.fromObservable { userRepo.updateEmail(state.newEmail) }.onEvent { status ->
  when (status) {
     SUCCESS -> transition { toastManager.showToast("Email updated!") }
     ERROR -> transition { toastManager.showToast("Email failed to update") }
  }
}

@mattkranzler5
Copy link
Contributor

I guess in my mind event is something that an action can produce/emit. For example, you can have an action which saves data to the backend and emits an event of success/failure status.

@Laimiux the way I read the above is that something from an Observable is going to produce RxActions. I think it also is a bit confusing for the Action.onData(...).onEvent {...} syntax. Action implies that anything passed into the builder will be an "Action" which can be a bit ambiguous. Whereas I feel like using Event implies whatever is passed into the builder will create Events. By consolidating to Event for both single use callbacks & emissions from a stream I think it could help with some of the naming complexity.

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 6e0a1f9 to e75ba9f Compare March 23, 2022 21:23
@johnwilde
Copy link
Collaborator

By consolidating to Event for both single use callbacks & emissions from a stream I think it could help with some of the naming complexity.

I like the idea of simplifying naming but I'd worry in this case that using the same term to refer to the callback and the emission would lead to more confusion. One downside to the rename is that in my mind Stream implied that it produced emissions whereas Action doesn't intuitively do that. However, I think I'll get used to it.

@Laimiux
Copy link
Collaborator Author

Laimiux commented Mar 23, 2022

I guess in my mind event is something that an action can produce/emit. For example, you can have an action which saves data to the backend and emits an event of success/failure status.

@Laimiux the way I read the above is that something from an Observable is going to produce RxActions. I think it also is a bit confusing for the Action.onData(...).onEvent {...} syntax. Action implies that anything passed into the builder will be an "Action" which can be a bit ambiguous. Whereas I feel like using Event implies whatever is passed into the builder will create Events. By consolidating to Event for both single use callbacks & emissions from a stream I think it could help with some of the naming complexity.

With RxAction.fromObservable we are creating a deferred action which will subscribe to an observable and emits events from it. In the above example, it subscribes to an observable which updates the email and emits modification status. I'm hesitant to unify Action and Event together because I see them as distinct things. Event being a notification/message/callback and action being a mechanism that does something (producing event being one of type of action). For example

  • Event<Event> would need to be Event<Data> or Event<T>
  • Callback definitions currently are (Event) -> Unit which will clash with the Event type.

I do agree that Action.onInit and Action.onData could have a better name. I would even think that maybe they should be

Action.perform { analytics.trackPageOpened() }
Action.perform(key = data) { analytics.trackDataChanged(data) }
Action.performOnTermination() { analytics.trackPageClosed() }
Action.emit(data).onEvent { 
  // if you need to do a state change when data changes 
  transition()
}

But, I think this is a bigger change that needs to be thought through more (around the API and consistency).

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch 7 times, most recently from 12499f8 to 63a49bb Compare March 23, 2022 22:51
@mattkranzler5
Copy link
Contributor

• Event would need to be Event or Event
• Callback definitions currently are (Event) -> Unit which will clash with the Event type.

I was thinking that Stream<Event> would become Events<Event> or Events<Type> and then the other "adapters" (RxStream, FlowStream could just become extension functions on Events so you could have something like:

Events.fromValue(...) {}
Events.fromObservable(...) {}
Events.fromFlow(...) {}

The naming makes more sense to me but I'll get used to whatever we decide.

@Laimiux Laimiux force-pushed the laimonas/naming-improvements branch from 63a49bb to 639902c Compare March 23, 2022 23:21
@Laimiux Laimiux merged commit 2447967 into master Mar 24, 2022
@Laimiux Laimiux deleted the laimonas/naming-improvements branch March 24, 2022 00:01
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.

None yet

8 participants