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

Add rememberEventSink #1074

Closed
wants to merge 1 commit into from
Closed

Add rememberEventSink #1074

wants to merge 1 commit into from

Conversation

stagg
Copy link
Collaborator

@stagg stagg commented Dec 21, 2023

Helper to control stability of extracted event sinks.

@stagg stagg marked this pull request as ready for review December 21, 2023 00:43
@stagg stagg requested a review from ZacSweers December 21, 2023 00:43
@chrisbanes
Copy link
Contributor

To me, this looks like more of a signal that we should separate out the CircuitState and events. Bundling in the event sink into the state makes implementation simpler, but we're conflating two different things here (which is why stability is an issue).

@ZacSweers
Copy link
Collaborator

Talked a little offline

  • @stagg is gonna write some tests to test some assumptions. We've heard downstream that compose-compiler automatically remembers lambdas in some (most? all? who knows!) cases that may make this redundant, but we've seen inconsistent behavior
  • Discussed a bunch with Adam and Bill separately and put up some prototypes of alternative state designs in Prototype different state designs #1077 if you wanna peek

@stagg
Copy link
Collaborator Author

stagg commented Jan 11, 2024

Going to verify this in a separate test PR.

@stagg stagg closed this Jan 11, 2024
@stagg stagg deleted the js-remember-eventsink branch January 11, 2024 19:35
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