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

Allow an optional CoroutineContext to be supplied to launchMolecule. #312

Merged
merged 7 commits into from Oct 25, 2023
Merged

Allow an optional CoroutineContext to be supplied to launchMolecule. #312

merged 7 commits into from Oct 25, 2023

Conversation

ghost
Copy link

@ghost ghost commented Oct 4, 2023

Allow an optional CoroutineContext to be supplied to launchMolecule.

This allows easier control of what dispatcher Molecule should run under in cases where we can't presume it's wired correctly in the provided CoroutineScope.

@ghost ghost marked this pull request as ready for review October 4, 2023 21:36
Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Can we write a test? You can add an arbitrary key to the context and assert it's present in the composition for something that works on all platforms.

@waylon-brown
Copy link
Contributor

Can we write a test? You can add an arbitrary key to the context and assert it's present in the composition for something that works on all platforms.

Added a test, and this also required an additional change here since the test made me aware I wasn't using the passed in context for the Recomposer.

@oldergod
Copy link
Contributor

oldergod commented Oct 6, 2023

Just in case, you might want to rebase on master for the apiDump task would fail otherwise after merge

@ghost
Copy link
Author

ghost commented Oct 6, 2023

Just in case, you might want to rebase on master for the apiDump task would fail otherwise after merge

Rebased onto master.

@ghost ghost requested a review from JakeWharton October 10, 2023 21:13
Comment on lines +27 to +28
public static final synthetic fun launchMolecule (Lkotlinx/coroutines/CoroutineScope;Lapp/cash/molecule/RecompositionMode;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function2;)V
public static final synthetic fun launchMolecule (Lkotlinx/coroutines/CoroutineScope;Lapp/cash/molecule/RecompositionMode;Lkotlin/jvm/functions/Function2;)Lkotlinx/coroutines/flow/StateFlow;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This diff and the tool that produces it are really subpar, but these two lines are the same as the ones which were deleted except with synthetic added (i.e., hiding them solely for binary compatibility). The new source-compatible versions are the two lines above and the synthetic functions for supporting their default argument values are the two lines below.

@JakeWharton JakeWharton enabled auto-merge (squash) October 25, 2023 15:08
@JakeWharton JakeWharton merged commit d80b7e6 into cashapp:trunk Oct 25, 2023
1 check passed
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