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

Implement CircuitContext #325

Merged
merged 9 commits into from
Dec 7, 2022
Merged

Implement CircuitContext #325

merged 9 commits into from
Dec 7, 2022

Conversation

ZacSweers
Copy link
Collaborator

@ZacSweers ZacSweers commented Dec 6, 2022

This introduces a new CircuitContext API to factories to replace the current CircuitConfig parameter.

The goal of CircuitContext is to offer a way to easily add extra context to presenter and UI factory requests and allow for extra flexibility to breadcrumb extra information via a tags API. CircuitContext is mutable in nature and modeled similarly to Moshi's JsonReader API, and should not be kept past factory creation.

Currently it exposes the global CircuitConfig and a tags API.

The tags API should allow consumers to plumb through anything extra they need that doesn't necessarily fit in Circuit's core API, such as telemetry infrastructure. This is similar in nature to the tags APIs found in Moshi, OkHttp, KotlinPoet, etc.

Along the way I've implemented some new EventListener APIs with access to this. Its test is now beefed up and demonstrates a nice example of a very basic logging listener.

22:25:57.090259: Creating new RecordingEventListener for com.slack.circuit.TestScreen@4829be4b
22:25:57.098234: onBeforeCreatePresenter: com.slack.circuit.TestScreen@4829be4b
22:25:57.098506: onAfterCreatePresenter: com.slack.circuit.TestScreen@4829be4b, com.slack.circuit.StringPresenter@1d33f9f7
22:25:57.098668: onBeforeCreateUi: com.slack.circuit.TestScreen@4829be4b
22:25:57.100256: onAfterCreateUi: com.slack.circuit.TestScreen@4829be4b, ScreenUi(ui=com.slack.circuit.StringUi@13251da5)
22:25:57.103494: onStartPresent
22:25:57.103796: onStartContent
22:25:57.104254: onState: StringState(value=State)
22:25:57.125711: onState: StringState(value=State2)
22:25:57.131911: onDisposeContent
22:25:57.132017: onDisposePresent
22:25:57.132055: dispose
22:25:57.132227: Removing RecordingEventListener for com.slack.circuit.TestScreen@4829be4b

@@ -391,11 +390,6 @@ private fun KSFunctionDeclaration.assistedParameters(
)
}
}
type.isInstanceOf(symbols.circuitConfig) -> {
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 shouldn't have been available as an assisted type anyway so no real loss here

Copy link
Collaborator

@kierse kierse left a comment

Choose a reason for hiding this comment

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

A couple problems I don't think this solution solves:

  1. There doesn't appear to be any way to get data from CircuitContext tags into a presenter or UI. At least not unless we want to manually build factories, which seems like a non-starter. This effectively means events occurring in Circuit can't be tied to events happening in the presenter or ui code.
  2. It doesn't appear possible to tie events for nested Circuits (ie calling CircuitConfig from UI logic) to the parent Circuit.

@kierse
Copy link
Collaborator

kierse commented Dec 7, 2022

We chatted offline about the following:

There doesn't appear to be any way to get data from CircuitContext tags into a presenter or UI. At least not unless we want to manually build factories, which seems like a non-starter. This effectively means events occurring in Circuit can't be tied to events happening in the presenter or ui code.

We can accomplish this via a delegating presenter implementation and factory that:

  1. pulls data out of the context tags
  2. creates the screen specific presenter
  3. exposes the data from step 1 via a composition local
  4. calls the present method

It doesn't appear possible to tie events for nested Circuits (ie calling CircuitConfig from UI logic) to the parent Circuit.

This was addressed by 79c7238

Copy link
Collaborator

@kierse kierse left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@ZacSweers ZacSweers merged commit 9941068 into main Dec 7, 2022
@ZacSweers ZacSweers deleted the z/circuitcontext branch December 7, 2022 20:42
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.

2 participants