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

types of fold #370

Closed
codingedgar opened this issue Sep 15, 2019 · 13 comments · Fixed by #374
Closed

types of fold #370

codingedgar opened this issue Sep 15, 2019 · 13 comments · Fixed by #374

Comments

@codingedgar
Copy link
Contributor

Hi !

The type of fold (or coalesce as proposed) is

export function fold<LA, RA, LB, RB>(lmapper: (left: LA) => RA, rmapper: (right: RA) => RB, source: FutureInstance<LA, RA>): FutureInstance<LB, RB>

I think it should be

export function fold<LA, RA, RB>(lmapper: (left: LA) => RB, rmapper: (right: RA) => RB, source: FutureInstance<LA, RA>): FutureInstance<never, RB>

ok, maybe the never shouldn't be there, but the lmapper seems wrong.

@Avaq
Copy link
Member

Avaq commented Sep 16, 2019

This issue was fixed in #335 and #336, I believe. Are you using the latest version of Fluture?

@Avaq
Copy link
Member

Avaq commented Sep 16, 2019

Ah, sorry. #336 was closed because it's a breaking change, and it has been fixed in the latest version

@codingedgar
Copy link
Contributor Author

I'm not using the beta version, what is beta about it? And what work is left before the next version? Maybe I can help. I've seen many fixes now in the 12 version.

@Avaq
Copy link
Member

Avaq commented Sep 16, 2019

what is beta about it

It's in beta because I wanted to accumulate all breaking changes for one major release. I plan to release it in the stable channel once all planned breaking changes, and Fluenture, are finished.

The code itself should be "bug-free" and usable. See upgrade guide for a list of breaking changes included so far. You can install this version with npm install fluture@beta.

Maybe I can help

The following are the remaining planned breaking changes to implement. You can help with these if you like:

For the completion of Fluenture, the following is required:

  1. Switch to c8 and oletus for testing  #368 Update the test suite so it works in Node 12
  2. Add testing utils to npm package #355 Export testing utils from Fluture, so Fluenture can use them to test its code
  3. Add everything fluenture#1 Add tests to Fluenture so the initial PR can be merged

@codingedgar
Copy link
Contributor Author

I stated with the beta version, and reading the upgrade guide, however when i do

pipe(
...,
fold<LA, RA, Error, RB>(..),
promise

It's not very clear how fold return FutureInstance<LB, RB> where the LB comes from? and fold is supposed to "coalesce" to RB but i'm having to return an Error, so i'm in some kind of deadlock here :(

@codingedgar
Copy link
Contributor Author

What about making fold

export function coalesce <LA, RB>(lmapper: (left: LA) => RB): <RA>(rmapper: (right: RA) => RB) => <LB>(source: FutureInstance<LA, RA>) => FutureInstance<never, RB>

I suppose that should be because some throw, like in the case of

See #331. In order to call promise on a Future, the rejection branch must now be compatible with type Error, and thrown exceptions are now merged with the rejection branch, instead of being rethrown.

but not sure if that's a problem of fold

Also in general add the generic <T> as late as possible has proven to be more effective, with the fluture@12.0.0-beta.3 i needed to type way more functions, specially fold and bimap. when i change bimap to

export function bimap<LA, LB>(lmapper: (reason: LA) => LB): <RA, RB>(rmapper: (value: RA) => RB) => (source: FutureInstance<LA, RA>) => FutureInstance<LB, RB>

ts infers way better the bimap type.

@Avaq
Copy link
Member

Avaq commented Sep 17, 2019

@edgarjrg I have not had any feedback regarding the usage of Fluture beta with TypeScript, so this is very valuable. It seems some of the problems you are experiencing have to do with the unused generics in many of these functions. I plan to get rid of these: #337 - so instead of using never I want to use unknown (or a type variable?).

Moving the generics into the nested functions where they are used seems like a great idea. I did not even know it's possible. Please feel free to submit a PR! :D

@Avaq
Copy link
Member

Avaq commented Sep 17, 2019

I created #372 from your comments. I think, if you have time, you can make a PR that tackles both #372 and #337 at the same time (but not yet #324, I'd like to tackle that in a separate PR). Let me know if you'd like to do this.

@codingedgar
Copy link
Contributor Author

Sure ! totally on board 👍 i really like this library.

@codingedgar
Copy link
Contributor Author

Why would the Left be unknown ? after coalesce the Left is well.. nothing, right ? Some kind of nil value, undefined might be closer to express that. I suggested never to express that is no longer necessary to deal with the rejection branch, kind of change from failable Fluture to unfailable Fluture. But that's debatable.

I guess that the point i want to make is that LB should be a default value (not generic), because there's no way in user land to put something in the left branch when coalescing.

@Avaq
Copy link
Member

Avaq commented Sep 19, 2019

Why would the Left be unknown ?
Some kind of nil value, undefined might be closer to express that.

Consider the following expression:

1 | reject (42) |>                    // Future<Number, ?>
2 | chain (x => resolve (`${x}!`)) |> // Future<Number, String>
3 | fold (x => x) (x => x.length) |>  // Future<?, Number>
4 | chain (_ => reject (null))        // Future<Null, Number>
  1. We create a rejected Future. We know the rejection type is Number, but we don't know the resolution type yet. The resolution type cannot be nil, because neither null or undefined are assignable to String in line 2.
  2. We chain over the rejected Future and now the type of the resolution branch becomes known: it is String. The type of the rejection branch stays a Number, because chain cannot change the rejection type. resolve (`${x}!`) returned a Future<?, String>, and so ? must be assignable to Number.
  3. We fold (coalesce) the two types into one: from String and Number to Number. We end up with the same situation we had on line 1, except this time it's the rejection type that's not known.
  4. Just as on line 2, we now chain with a Future that has a determined rejection type Null. Null must be assignable to ?, because as we know, chain cannot change the rejection type.

Anywhere you're seeing ?, we have to use some type. In Haskell, this would be represented by a non-determined type variable. I've tried to simulate this by using generics in these places, allowing the user to tell TypeScript what type it should be. We cannot use null, because it's not assignable to whatever other type the user might chain with. We cannot use never for the exact same reason (see #230). TypeScript recently added the unknown type, which seems to fit this problem perfectly.

Anything is assignable to unknown, but unknown isn’t assignable to anything but itself
-- TypeScript 3.0 changelog


there's no way in user land to put something in the left branch when coalescing

Only if you consider coalesce in isolation. Combined with chain, a user can, as demonstrated. Does this answer your question?

@codingedgar
Copy link
Contributor Author

codingedgar commented Sep 20, 2019

Thank you for your explanation 🙏

The properties of undefined are what we need (to be able to be assigned to other types), but due to --strictNullChecks and semantics is inappropriate to use it, therefore we should find a solution in unknown, any, {}, never, other?, it appears that unknown is the best bet.

I added dtslint to check types in this commit(It's very clumsy, mostly prove of concept/setup to add tests to types, sorry) this way we can add expressions and test if the types behave as expected, please run npm dtslint to check the results/errors.

I hope we could add all expressions we would like to be possible in the tests, I used an approach of chain, chain1, chain2 for method types variants, maybe there's a better way to do so? With this approach chanN don't map to chain, but the tests pass, the values would give an error though.

As far as i got, never wins every time for the expressions we talked and unknown is not useful at all. I think that this should be the opposite (am i doing something wrong?) when i'm writing client code, ts inference assigns unknown to not determined types, this is 👌 , but if i do type L as unknown instead of a generic the compiler behaves completely different, maybe the inferred unknown is no the same as unknown. Still much to learn from ts but it's very possible that's the case.

  1. Notice that i had to use // TypeScript Version: 3.6 because of this issue but // TypeScript Version: 3.0 should be fine.
  2. Maybe i'm doing wrong using external libraries, but i added them for readability and integration. I found needing both very quickly.
  3. There're default tslint rules disabled, in particular no-unnecessary-generics is direct to Stop using user-assignable type generics in the types of functions that don't do anything with them #337
  4. There're other tslint rules disabled, i started to please dtslint but too many changes at the same time are not helpful.
  5. There's index.d.ts and types/index.d.ts, i think we should use types/index.d.ts to help dtslint and make the changes necessary to the package.json .. i just didn't.. yet and maybe will end up using other stuff or nothing.
  6. method is the original type
  7. method1 is the lazy generic Move TypeScript generics as deeply into function signatures as possible #372
  8. method2 is replacing generic with undefined
  9. method3 is replacing generic with unknown
  10. method4 is replacing generic with never
  11. method5 is replacing generic with never and some union types, that we discussed shouldn't be used, but i couldn't resits to try 😇
  12. fold as current does not work well for me, maybe i'm doing something wrong/bad/unintended use
    image
  13. fold with never is 👌
    image
    image
  14. fold with unknown does not work either
    image
  15. I expected the behavior of never and unknown to be the other way around really.

I also looked into npm-ramda which uses a similar approach to validate types and tests types+returns at the same time, pretty neat, maybe we could do something like that.

I failed to understand #230 , i tried to recreate the scenario but the tests do not seem to benefit at all with that change, maybe the real code has the noted exception but the given example works fine with never instead of L. ts compiler might be better now also.

@Avaq
Copy link
Member

Avaq commented Sep 22, 2019

Let's continue the discussion about never/unknown in the relevant issue: #337 (comment)

Avaq added a commit that referenced this issue Sep 25, 2019
- Revert #230 and reintroduce the usage of the 'never' type
- Relax the signature of chain and chainRej to allow TypeScript to
  derive types when merging a Future of a never with a Future of a
  real type. Closes #337 and closes #370.
- Move generic types closer to where they are used in the function
  signature. Closes #372 and closes #373 through supersession.

Co-authored-by: Edgar Rodriguez <edgarj.rodriguezg@gmail.com>
@Avaq Avaq closed this as completed in #374 Sep 26, 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

Successfully merging a pull request may close this issue.

2 participants