Skip to content

Conversation

@Frugghi
Copy link
Contributor

@Frugghi Frugghi commented May 27, 2025

A common pattern is to create a Diffuser that is a combination of several diffusers that map individual properties of the model:

let diffuser: Diffuser<Model> = .intoAll([
    .map(\.propertyOne, .into { /* do something with propertyOne */ },
    .map(\.propertyTwo, .into { /* do something with propertyTwo */ }
])

This works fine except that it forces you to make the model Equatable because intoAll requires the object to be Equatable. When using this pattern, you don't care about the model being Equatable but only about the individual properties being Equatable, in fact the entire object will be cached by intoAll and then each property will be cached by into.

To avoid this unnecessary conformance to Equatable, this PR introduces a variant of intoAlways that accepts a list of diffusers and always run them making the previous example:

let diffuser: Diffuser<Model> = .intoAlways([
    .map(\.propertyOne, .into { /* do something with propertyOne */ },
    .map(\.propertyTwo, .into { /* do something with propertyTwo */ }
])

Since the individual properties are still Equatable as required by .into, the two examples above are pretty much equivalent.

Comment on lines 36 to 37
/// This function is useful as a building block for more complex Diffusers, but it is unlikely that you would use
/// it on its own. Consider using `into` instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this sentence still relevant in those two new variants of intoAlways?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, perhaps you could document briefly the intended use case of intoAlways for non-Equatable models with Equatable members.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! I think that sentence would still apply in this case but it's not really that clear so I replaced it to point out about the existence of intoAll instead.

Copy link
Collaborator

@zvonicek zvonicek left a comment

Choose a reason for hiding this comment

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

One trade-off of this PR is that when the base model is Equatable, there are now two ways of expressing the identical logic, by using .intoAlways or .intoAll. Ideally, I would like the library to be more opinionated and prevent such a state, but I understand that there are limited ways of doing so at the moment without causing a BC break.

I, on the other hand, quite like the .intoAlways solution for dealing with non-Equatable base models, which seems like an elegant way to incorporate this much needed capability into the library.

So I think that we can go ahead with merging this one!

@zvonicek
Copy link
Collaborator

cc @togi in case you have any thoughts on this before we merge it

@Frugghi
Copy link
Contributor Author

Frugghi commented May 29, 2025

One trade-off of this PR is that when the base model is Equatable, there are now two ways of expressing the identical logic, by using .intoAlways or .intoAll.

I get the point but I don't think it's any different from .intoAlways vs .into. They express identical logic only if you combine them with something like map + into like in the examples I provided.

@zvonicek zvonicek merged commit fea166a into spotify:master Jun 2, 2025
1 of 2 checks 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.

2 participants