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

Should we adopt .map, .flatMap terminology? #5

Closed
lilyball opened this issue Jan 15, 2018 · 4 comments
Closed

Should we adopt .map, .flatMap terminology? #5

lilyball opened this issue Jan 15, 2018 · 4 comments
Milestone

Comments

@lilyball
Copy link
Owner

Right now we use .then, .catch, .recover, and .always. Should we adopt more functional terminology, such as .map, .flatMap, .mapError, etc? This would mean less overloading.

@lilyball lilyball added the question Further information is requested label Jan 15, 2018
@hborders
Copy link

I vote for .map, .flatMap, etc.

@abhisood
Copy link

+1 for .map, .flatMap, etc.

@lilyball
Copy link
Owner Author

I'm still conflicted about this.

Pros of new terminology:

  • Avoiding overloads can help the compiler typecheck without explicit type annotations.
  • Avoiding overloads also avoids cases where the compiler produces stupid errors.
  • It's a bit more explicit about when you're returning a nested promise vs not.
  • With the new terminology we can then easily add more variants like mapError that I don't have a good name for in the current terminology.
  • We can go ahead and keep the then and catch names as versions where the block doesn't return anything, making it a bit more convenient to attach observers that don't mutate the value (they'd still return a new promise, to enforce the ordering assumptions we make about chained promises, but the new promise would adopt the same value as the receiver).

Cons of new terminology:

  • It increases the visible API surface, which can make it harder to remember the method you want. Right now if you want to attach an observer for the success case it's then, regardless of whether you're returning a value or a new promise. This is already confusing enough in the Obj-C API where we have separate names due to lack of overloading (e.g. -inspect vs -always).
  • On a similar note, it makes it easier to accidentally end up with a Promise<Promise<T>> because you wrote .map instead of .flatMap. The worry here is that someone not familiar with the library wouldn't realize that using .flatMap to flatten the promises is an option. Perhaps I could add a deprecated override for this case that tells the user to use .flatMap instead, except then if the user really did want a Promise<Promise<T>> they couldn't (though I don't know why they'd ever want that).
  • Probably the biggest issue is the fact that I'm not sure what to do about the threading model. The current callbacks, when run without a context, use the .auto context which means to use .main if set up on the main queue. I'm concerned that this behavior may not make sense for e.g. .map. But I also don't want different methods to default to different threads. The only reasonable solution I can think here is to remove the default value for any callbacks that return a value (.map, .flatMap, etc.) and only allow the caller to skip the context for the "terminal" observers (.then, .catch, etc). The idea behind the default contexts was to allow people to incrementally opt in to more complexity/control, and removing the defaults requires the caller to think about it, but maybe that's a good thing.

@hborders
Copy link

I think you spelled out the pros and cons very well.

@lilyball lilyball added this to the Next milestone Feb 16, 2019
@lilyball lilyball removed the question Further information is requested label Feb 16, 2019
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

No branches or pull requests

3 participants