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

Synthetic fold #30

Closed
fes300 opened this issue Jan 29, 2019 · 7 comments · Fixed by #53
Closed

Synthetic fold #30

fes300 opened this issue Jan 29, 2019 · 7 comments · Fixed by #53

Comments

@fes300
Copy link
Contributor

fes300 commented Jan 29, 2019

hi @raveclassic,
first of all, thank you for this piece of code, it defines a very complete and clean interface for dealing with IO data.

In most of the apps I design, when I fetch data the initial case does not need to be handled (data is fetched as soon as a view is loaded so I want to handle only the loading, failure and success cases). As this is true for 95% of my use cases, it seems a bit overkill to fold on my data structures as it forces me to handle separately the loading and initial case but at the same time, I would like to keep the strictness for all other cases.
How would you feel about a syntheticFold method that merges the handling of initial and loading?

@raveclassic
Copy link
Contributor

@fes300 Well this is completely related to #10

@fes300
Copy link
Contributor Author

fes300 commented Jan 29, 2019

totally, I wrote my comment there you can close this 👍

@smaccoun
Copy link

smaccoun commented Aug 3, 2020

While I agree with that issue that we should keep initial, still seems to me it would be useful to have this syntheticFold idea you propose for situations when we want to treat initial and pending as identical. Does that method exist? If not, this issue still feels relevant as #10 is debating removing it, while this issue could be about providing a utility method to improve ergonomics for a "common case"

@raveclassic
Copy link
Contributor

raveclassic commented Aug 3, 2020

So there're 2 ways of implementing such fold:

export const fold3 = <E, A, R>(
	onNone: () => R,
	onFailure: (e: E) => R,
	onSuccess: (a: A) => R,
): ((fa: RemoteData<E, A>) => R) => fold(onNone, onNone, onFailure, onSuccess);

export const fold3_ = <E, A, R>(
	onNone: (progress: Option<RemoteProgress>) => R,
	onFailure: (e: E) => R,
	onSuccess: (a: A) => R,
): ((fa: RemoteData<E, A>) => R) => fold(() => onNone(none), onNone, onFailure, onSuccess);

And therefore we have 3 questions:

  • which one should we pick?
  • what name should we pick? fold3 looks weird
  • is it worth adding such one-liner to the core?

@smaccoun
Copy link

smaccoun commented Aug 3, 2020

which one should we pick?

I'm in favor of the latter if we have to pick one since it provides more capability, but I also think there's a case to be made that both could be useful. If we support both, maybe we could rename the parameters to onPending for fold3 and onProgress for fold3_?

what name should we pick? fold3 looks weird

I was wondering the same. I wonder if it would be nice to just be fairly verbose/explicit and name it something like foldPendingOrReplete for fold3 and foldProgressOrReplete for fold3_, but I admit those feel a little clunky. We could also call it foldIgnoreInitial, but tbh I could live with a simple name like fold3 as I think it is still fairly clear what it does.

is it worth adding such one-liner to the core?

IMO yes. Although it's simple and everyone could add it to their own prelude in their own project, given #10 suggests this situation is quite common for most people, I think it should just be part of this lib. But if it feels clunky for some reason, I'm open to the counterargument

@fes300
Copy link
Contributor Author

fes300 commented Aug 6, 2020

Hi everyone, nice to see this picked up again :)
here are my 2cents:

which one should we pick?

I think the second implementation maps better to current implementations / doesn't remove potentially important functionality

what name should we pick? fold3 looks weird

I kinda like it actually, it has a "mathy" flavour :)

is it worth adding such one-liner to the core?

I still think this is relevant for the ergonomics of the package, I think it is important enough to live in the core

@raveclassic
Copy link
Contributor

Ok guys, feel free to send a PR for this :)

@fes300 fes300 mentioned this issue Sep 11, 2020
raveclassic pushed a commit that referenced this issue Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants