Skip to content

Require a return value for all transformative functions#190

Open
jleider wants to merge 3 commits into
monet:developfrom
jleider:return-types
Open

Require a return value for all transformative functions#190
jleider wants to merge 3 commits into
monet:developfrom
jleider:return-types

Conversation

@jleider

@jleider jleider commented Oct 25, 2018

Copy link
Copy Markdown
Contributor

This enforces that calls to map, leftMap, cata, etc. provide a return value where it is expected.

One caveat to note is that users will find that side-effectual code will now throw type errors. For example, we use cata / fold fairly extensively in our codebase. Some of the cases we had side-effectual functions that didn't have a return value. These now need to be wrapped in IO. Additionally, there could be a convenience function that would operate on both left/right, some/none, etc. like cata but for side-effects similar to forEach since currently there is no way to chain forEach and orElseRun because they both return void instead of this.

Requires TypeScript 2.8+.

@ulfryk ulfryk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanc
ks for contribution!

  1. We shouldn't enforce TS higher than 3.0.x in 0.9.0 so this PR wil wait till post release
  2. This should also include all flatMaps, chains, binds and catchMaps

👍

Comment thread package.json
"karma-jasmine": "1.1.2",
"karma-mocha-reporter": "2.2.5",
"typescript": "3.0.3",
"typescript": "3.1.3",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be nice to not enforce upgrade to TS higher than 3.0

// uncomment to fail typecheck for non return value
// twelve.map(() => { "no return"; });
// twelve.leftMap(() => { "no return"; });
// twelve.cata(() => { "no return" }, () => { "no return"; });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cata should allow to return void

@ulfryk

ulfryk commented Oct 26, 2018

Copy link
Copy Markdown
Member

Additionally, there could be a convenience function that would operate on both left/right, some/none, etc. like cata but for side-effects

Use of .cata for side effects is completely valid (in JS world), but we may consider explicitly forbidding to return void in it's callbacks, and adding separate operators for this purpose.

I'd just not allow chaining - call to side effects should e the last thing done ;)

Optionally we may introduce UNSAFE_forEach, UNSAFE_forEacLeft UNSAFE_forEach…catalikeHoweverWeCallIt…

WDYT @jleider ?

@ulfryk

ulfryk commented Jun 4, 2019

Copy link
Copy Markdown
Member

@jleider - is there a chance you'd continue on this?

@jleider

jleider commented Jun 4, 2019

Copy link
Copy Markdown
Contributor Author

@ulfryk Unfortunately not, we migrated to https://gcanti.github.io/fp-ts/ even though they don't provide a forEach vs map either. FP-TS aligns more closely with our functional server side libraries.

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