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

Allow Reader's local to infer the environment type #1761

Open
wants to merge 1 commit into
base: 3.0.0
Choose a base branch
from

Conversation

thewilkybarkid
Copy link
Contributor

@thewilkybarkid thewilkybarkid commented Sep 2, 2022

Using local from Reader recently, I found it a bit surprising that I had to redeclare the type even when I didn't change it.

Looking at #892, I've replicated the generics from fp-ts 1: by default, it treats the type as the same. (If you're changing it, you have to declare it anyhow.)

Reversing the order might mean this should wait for fp-ts 3...

@gcanti
Copy link
Owner

gcanti commented Sep 2, 2022

Since the actual goal of local is changing the environment type, deep down, isn't a good thing you had to redeclare?

thewilkybarkid added a commit to PREreview/prereview.org that referenced this pull request Sep 5, 2022
We can update the metadata on Zenodo records, but this doesn't often happen. This change means we use stale versions from the HTTP cache but revalidate them in the background. As a result, the page will load faster but might temporarily contain out-of-date information.

The behaviour is similar to if Zenodo were to return an HTTP response with a stale-while-revalidate directive.

Note that the 'local' function from fp-ts can't infer the argument type, so I've used function overloading to avoid having to declare it in multiple places.

Refs #249, https://doi.org/10.17487/RFC5861, gcanti/fp-ts#1761
@thewilkybarkid
Copy link
Contributor Author

PREreview/prereview.org@f76f852 is my use case: I have functions that manipulate the environment, but they have to be typed explicitly rather than relying on extends. It's ok at the moment as it's a short list of known possibilities so I can overload, but allowing inference would solve it.

If you are changing at the type, you do still have to declare it.

@gcanti
Copy link
Owner

gcanti commented Sep 8, 2022

Reversing the order might mean this should wait for fp-ts 3...

What about adding an overloading instead of reversing the order? Should be backward compatible

// Reader.ts

export function local<R>(f: (r: R) => R): <A>(ma: Reader<R, A>) => Reader<R, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> {
  return (ma) => (r2) => ma(f(r2))
}

@thewilkybarkid
Copy link
Contributor Author

@gcanti Took a bit of trial and error to get overloading to work also with my use (I've added more tests to cover it).

@gcanti
Copy link
Owner

gcanti commented Sep 8, 2022

@thewilkybarkid I tried adding your tests locally and they seems to work with

export function local<R>(f: (r: R) => R): <A>(ma: Reader<R, A>) => Reader<R, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> {
  return (ma) => (r2) => ma(f(r2))
}

what's the reason of...

// ..this? ----------v-----------v
export function local<R1, R2 = R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> {
  return (ma) => (r2) => ma(f(r2))
}

@thewilkybarkid
Copy link
Contributor Author

thewilkybarkid commented Sep 8, 2022

You're right, they do work with the simpler version. I also checked it against my project, which caused a failure in my 'normal' usage elsewhere (i.e. changing the environment type), https://github.com/PREreview/prereview.org/blob/eebd28e6b220dd7d8464eff58d7111e6b3234b7f/src/app.ts#L81 (R being fp-ts-routing; local from Reader though it's actually a ReaderMiddleware from hyper-ts). I'll see if I can isolate what's happening there.

Edit: Seems to be something to do with the spread operator (I'm trying to add a new property to the environment locally).

@thewilkybarkid
Copy link
Contributor Author

thewilkybarkid commented Sep 8, 2022

// $ExpectType Reader<{ b: string; }, string>
pipe(
  _.of<{ a: string; b: string }, string>('a'),
  _.local((env: { b: string }) => ({
    ...env,
    a: env.b
  }))
)

is an example of a test case that fails with the simplified version: it infers Reader<{ a: string; b: string; }, string> even though { b: string } is typed.

@gcanti
Copy link
Owner

gcanti commented Sep 8, 2022

Good catch.

However

export function local<R1, R2 = R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> {
  return (ma) => (r2) => ma(f(r2))
}

is not backward compatible because AFAIK the second export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> overloading will never be used.

For example the following code

pipe(
  _.of<string, number>(1),
  _.local<boolean, string>((b) => (b ? 'a' : 'b'))
)

compiles fine with [email protected] and also with

export function local<R>(f: (r: R) => R): <A>(ma: Reader<R, A>) => Reader<R, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A>
export function local<R2, R1>(f: (r2: R2) => R1): <A>(ma: Reader<R1, A>) => Reader<R2, A> {
  return (ma) => (r2) => ma(f(r2))
}

but raises and error after the proposed change

@thewilkybarkid
Copy link
Contributor Author

I suspect the only option left is to target v3.

@gcanti gcanti added this to the 3.0.0 milestone Sep 8, 2022
@thewilkybarkid thewilkybarkid changed the base branch from master to 3.0.0 September 8, 2022 13:17
thewilkybarkid added a commit to PREreview/prereview.org that referenced this pull request Dec 20, 2022
The timeoutRequest decorator didn't need overloading as the position of the generic saw TypeScript infer the type correctly. This change wraps the other decorators in a function so they can take advantage of this technique.

Refs #536, f76f852, gcanti/fp-ts#1761
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