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

TypeScript support #431

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

TypeScript support #431

wants to merge 27 commits into from

Conversation

davidchambers
Copy link
Member

🚧 πŸ‘·β€β™€οΈ 🚧

Partially addresses #254

This is a work in progress, but as people are already providing useful feedback it makes sense to open a pull request for ongoing discussion.

@davidchambers
Copy link
Member Author

Thank you very much for your feedback, @tycho01. πŸ‘

binary fantasy-land methods lack currying

None of the methods specified by Fantasy Land is curried, so I believe this to be correct.

Why are there A generics when not used?

I wanted the specialized signatures to resemble the general signatures as closely as possible, and assumed the unused type variable would simply be ignored. Is my assumption correct?

In current TS I can't even think of a good fix to make this DRY again.

I've resigned myself to the file being at least 1000 lines. I'm not overly bothered by this, provided we having tooling in place to catch errors. Based on the comment from @ikatyang we should be able to achieve this.

Array typing on map precludes input from alternative implementations including Immutable.js's List.

This is faithful to Sanctuary's behaviour. Any type with a fantasy-land/map method is supported, as are Array, Object, and Function for which implementations are provided by sanctuary-type-classes.

In parseJson<A, B>(pred: (A) => boolean, s: string): Maybe<B>, the A generic is completely counter-productive.

In accordance with #424 the A should be changed to any. I don't understand the rest of your concerns about parseJson. I should be clear that the day I've spent working on this pull request so far is the only experience I have with TypeScript to date.

From your parseJson usage example though, the predicate appears not to have parameters?

The predicate takes one argument.

index.d.ts Outdated
}
interface Alternative<A> extends Applicative<A>, Plus<A> {
constructor: ApplicativeTypeRep<A> & PlusTypeRep<A>
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unhappy with this definition. I had hoped that

interface Alternative<A> extends Applicative<A>, Plus<A> {}

would suffice, but the constructor property requirements are seen to conflict rather than combine.

Is there an elegant solution to this problem?

Choose a reason for hiding this comment

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

for what it's worth, I'm not currently aware of any.

Choose a reason for hiding this comment

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

Actually, you could probably abstract it into a pattern I guess. But yeah only really pays off given multiple occurrences of this.

@KiaraGrouwstra
Copy link

I wanted the specialized signatures to resemble the general signatures as closely as possible, and assumed the unused type variable would simply be ignored. Is my assumption correct?

Yeah, you're right, they are.

Based on the comment from @ikatyang we should be able to achieve this.

Yeah, agreed, their approach looks good there. For Ramda typings it added conciliating with run-time tests over typings-checker, which in turn helped protect against any inference over just running the TS compiler.

This is faithful to Sanctuary's behaviour.

That's what one might assume looking at the Ramda docs as well. But in practice they just iterate over arrays using for (var i = 0; i < arr.length; i++), which any ArrayLike supports in exposing length and the numerical index. Won't hold if you're actually using Array.prototype though obviously.

In accordance with #424 the A should be changed to any. I don't understand the rest of your concerns about parseJson.

B is only used in the return type position, meaning the only way for it to get a type is if the user were to explicitly specify it in the generics. At that point, the user may find it an annoyance they'd be made to manually write out an additional type for the predicate as well. But yeah, if you make it any and ditch the generic, that'd solve it.

I'll concede that may make for a trade-off between TS UX and compliance with docs. I guess that was a reason I kinda gave up on trying to make them match for the Ramda typings -- I never really tried since the guys before me hadn't either, but I fear it may be tough to always keep the number of generics equal. These two examples aside, in TS I've had the impression it's easy to end up with more. Maybe for UX here having it say e.g. pred: (a: any) => boolean could do to invoke the a connection with the docs, I dunno.

I should be clear that the day I've spent working on this pull request so far is the only experience I have with TypeScript to date.

Well, I'd gotten type-classes wrong on my try, so I think you're doing pretty good! πŸ˜ƒ

index.d.ts Outdated
export function equals<A>(x: Error): (y: Error) => boolean;
// XXX: This is too general. Can we match "plain" objects only?
// export function equals<A>(x: Object, y: Object): boolean;
// export function equals<A>(x: Object): (y: Object) => boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion, @tycho01?

Choose a reason for hiding this comment

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

There's object and {}. I haven't even bothered to look into their nuances. Object appears not used as it'd match almost anything.

On DRY'ing... equals<A>(x: any, y: any): boolean? πŸ˜…
Similar to the lt / gt; wanted to say there's equals<A extends null | undefined | boolean | ...>(x: A, y: A): boolean (or even drop the constraint), but... generics don't just fail with literals here, it just does nothing useful. At that point it'll just be like, equals(123, "foo") totally works for A: any (or for number | string).
So technically without run-time type checks equals(123, "foo") may still run and produce false I guess?
I'd wanna go for a type-level type check, but those aren't possible yet. Failing that, it's your solution or any. If you'd consider not doing run-time errors for different types, with your approach you might even add a equals<A>(x: any, y: any): false fallback at the bottom.

Copy link
Member Author

Choose a reason for hiding this comment

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

On DRY'ing... equals<A>(x: any, y: any): boolean?

This is feasible for S.equals but not for S.map and other polymorphic functions which can operate on values of type StrMap a (plain objects with consistently typed values).

At that point it'll just be like, equals(123, "foo") totally works for A: any (or for number | string).

That's unfortunate. Although sanctuary-def provides $.Any this is not considered to be a "unifying" type. It's a pity that TypeScript can't express types such as a -> a -> a.

If you'd consider not doing run-time errors for different types, with your approach you might even add a equals<A>(x: any, y: any): false fallback at the bottom.

Is it possible to include implementations in a .d.ts file? Keep in mind that the implementation is still written in a .js file.

Choose a reason for hiding this comment

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

What you could probably do is store the type using the generic as a generic type, then type the function using an interface of overloads populated by instances of this generic type, which would... maybe look a bit cleaner. I never tried anything like it though.

type Equals<A> = (x: any, y: any): boolean; then like interface equals { Equals<string>; ... }

I'm on phone atm so can't test well, pretty sure it'd fail but it's the best improvement I can come up with. So probably just gonna be the way you got it now then.

Choose a reason for hiding this comment

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

For map consider checking the definition used in the Ramda typings.
What do you mean on the .d.ts vs. js?
Putting JS in a .d.ts file, I forgot if I tried, so don't take my word for it, but it probably fails.
Having js in the .d.ts seems not to make much sense though, so I'm probably misinterpreting what you mean by implementation.

@davidchambers
Copy link
Member Author

This is faithful to Sanctuary's behaviour.

That's what one might assume looking at the Ramda docs as well.

Sanctuary has no notion of "array-like" objects:

S.map(S.toUpper, {'0': 'foo', '1': 'bar', '2': 'baz', length: 3});
// ! TypeError: Type-variable constraint violation
//
//   map :: Functor f => (a -> b) -> f a -> f b
//                                   ^^^
//                                    1
//
//   1)  {"0": "foo", "1": "bar", "2": "baz", "length": 3} :: Object, StrMap ???
//
//   Since there is no type of which all the above values are members, the type-variable constraint has been violated.

If one evaluates this expression with Sanctuary's run-time type checking disabled, S.map will map over what it assumes to be a value of type StrMap String, and a run-time exception will be thrown when attempting to invoke toUpperCase on 3.

index.d.ts Outdated
(a: A, b: B, c: C): R
(a: A, b: B): (c: C) => R
(a: A): AwaitingTwo<B, C, R>
}
Copy link
Member

Choose a reason for hiding this comment

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

I've seen these before somewhere. πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

...and they may be problematic: #431 (comment) :(

index.d.ts Outdated

export function bimap<A, B, C, D>(f: (A) => B, g: (C) => D, bifunctor: Bifunctor<A, C>): Bifunctor<B, D>;
export function bimap<A, B, C, D>(f: (A) => B, g: (C) => D): (bifunctor: Bifunctor<A, C>) => Bifunctor<B, D>;
export function bimap<A, B, C, D>(f: (A) => B): AwaitingTwo<(C) => D, Bifunctor<A, C>, Bifunctor<B, D>>;

Choose a reason for hiding this comment

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

Notice that TS won't infer types across the function call, this kind of function should be something like:

-export function bimap<A, B, C, D>(f: (a: A) => B): AwaitingTwo<(c: C) => D, Bifunctor<A, C>, Bifunctor<B, D>>;
+export function bimap<A, B>(f: (A) => B): {
+  <C, D>(x: (c: C) => D, y: Bifunctor<A, C>): Bifunctor<B, D>;
+  <C, D>(x: (c: C) => D): (y: Bifunctor<A, C>) => Bifunctor<B, D>;
+};

Otherwise the C and D will be inferred as {}, since they are not exised in the first function call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @ikatyang! I will make this change and similar changes then push another commit. I'd love you to review it. :)

Choose a reason for hiding this comment

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

I have some tools to restructure this kind of definition in my types-ramda, but it does not expose yet, I'll expose them ASAP to make restructure FP definitions easily.

Choose a reason for hiding this comment

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

And it's recommended to setup a test first, since our eyes can't catch all the bugs, but tests can.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been playing with the fully applied form of S.bimap to understand your feedback, but TypeScript seems oblivious to type errors:

S.bimap(S.toUpper, Math.sqrt, S.Left(42))

Am I doing something wrong, or is TypeScript simply unifying everything to any?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps you could pull this branch and run these commands:

$ mkdir experiment
$ cd experiment
$ echo $'import * as S from "..";\n\nS.bimap(S.toUpper, Math.sqrt, S.Left(42));' >index.ts
$ tsc index.ts
$ cat index.js
"use strict";
exports.__esModule = true;
var S = require("..");
S.bimap(S.toUpper, Math.sqrt, S.Left(42));

Do you observe the same behaviour?

Choose a reason for hiding this comment

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

OK, I know what you mean. The current branch haven't updated the (A) => B to (a: A) => B, so that A is considered a variable name and its type is any.

Choose a reason for hiding this comment

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

You can copy code from the playground link I mentioned above, and just put it in test.ts then tsc test.ts, you'll see the error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current branch haven't updated the (A) => B to (a: A) => B, so that A is considered a variable name and its type is any.

Aha! I overlooked that in your first comment. Thank you for being patient with me. :)

Copy link

Choose a reason for hiding this comment

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

@davidchambers I've done my dts-element-fp for restructuring curried function definitions (placeholder available), might be useful to you.

index.d.ts Outdated
// Placeholder

interface Placeholder {
'@@functional/placeholder': boolean
Copy link
Member

@Avaq Avaq Aug 2, 2017

Choose a reason for hiding this comment

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

I think you could simply use true instead of boolean. Wouldn't want Sanctuary to treat my value as a placeholder even though I explicitly stated that @@functional/placeholder is false!

Copy link
Member Author

Choose a reason for hiding this comment

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

true is not a type, though, is it?

Copy link

Choose a reason for hiding this comment

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

true is a literal type.

literal type: boolean (true, false), number(1, 2, etc.) and string ('str1', 'str2', etc.)

Copy link
Member Author

Choose a reason for hiding this comment

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

If I change boolean to true, this code example does not type check:

import {concat} from 'sanctuary';

const _ = {'@@functional/placeholder': true};

concat(_, 'def')('abc');

Do you know what's wrong, @ikatyang? I'd love to use true if possible. :)

Copy link

Choose a reason for hiding this comment

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

For objects, TS always widen their type to non-literal, so in this case you have to assert it to true manually, e.g.

const _ = {'@@functional/placeholder': true as true}; //=> {'@@functional/placeholder': true}
const _ = {'@@functional/placeholder': true}; //=> {'@@functional/placeholder': boolean}

Copy link
Member Author

Choose a reason for hiding this comment

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

That works! Thank you for the explanation. I'm enjoying learning from you. :)

@miangraham
Copy link

miangraham commented Aug 30, 2017

Interested party here. Any ETA on a merge? Any specific help needed?

I'm looking at giving Sanctuary a spin from a TS project and adding it to an "How to Do Immutable Typescript" starter package I'm working on. Can work directly from the davidchambers/typescript branch for a bit if I need to. Thoughts appreciated.

@davidchambers
Copy link
Member Author

Interested party here. Any ETA on a merge? Any specific help needed?

Thank you for taking an interest, @miangraham, and for submitting a helpful pull request. I don't know when this pull request will be merged, as I'm focused on sanctuary-js/sanctuary-scripts#1 at present.

This pull request is not blocked, as far as I recall, but there's still a lot to do:

  • add type definitions for remaining functions;
  • programmatically assert that the type definitions don't introduce any test failures; and
  • add documentationβ€”or documentation linksβ€”alongside type definitions.

@miangraham, if you're interested in contributing to this effort I will give you write access so you can push commits directly to davidchambers/typescript. :)

@miangraham
Copy link

  • programmatically assert that the type definitions don't introduce any test failures

@davidchambers This seems like good bang for buck and a spitball should be super easy. Something like davidchambers/typescript...miangraham:davidchambers/typescript could hit index.d.ts with strict type checking, tslint, and some new type-level tests ("this line should fail to type check in this way") from your existing hooks.

Can def chip in some type defs too. Feel free to add me and I can try pushing a few things your way over the weekend.

@davidchambers
Copy link
Member Author

Feel free to add me and I can try pushing a few things your way over the weekend.

Excellent! Feel free to push to this branch. Please don't --force push as it's now a collaborative branch.

@miangraham
Copy link

miangraham commented Aug 31, 2017

The last CircleCI pass included these:

make lint
# (type check index.d.ts but not tests b/c tests have type errors by design)
node_modules/.bin/tslint \
	  --type-check \
	  --project tsconfig.json \
	  -- index.d.ts
node_modules/.bin/tslint \
	  --project test/typescript/tsconfig.json \
	  -- test/typescript/*.ts

make test
node_modules/.bin/typings-checker --allow-expect-error --project test/typescript/tsconfig.json test/typescript/*.ts
test/typescript/map.ts: 2 / 2 checks passed.

For a tslint rules baseline I started with tslint-config-standard and turned off everything needed for index.d.ts to pass as-is. Big ones to consider turning back on:

@miangraham
Copy link

miangraham commented Sep 2, 2017

For reference, current obvious index.d.ts TODOs:

  • fantasyland
    • extend
    • extract
    • contramap
    • filter
    • filterM
    • takeWhile
    • dropWhile
  • Function
  • Composition
  • Maybe (has interface, needs functions)
  • Either
  • Logic
  • List
  • Array
  • Object
  • Full currying support: curry4/5, flip, compose, on, ifElse

I got filter (with tests) working for Array without issue then bounced off the interface for non-arrays (I'm new to Fantasy Land). Will come back to it. In the meantime, Function and Logic look straightforward so I'll try grinding through those for a win.

@KiaraGrouwstra
Copy link

Monads aside there might well be overlap with ramda, so feel to take from there what you need.

@miangraham
Copy link

miangraham commented Sep 2, 2017

Many-param curryN plus placeholders is a bit nuts. Automation may be the only way to preserve sanity. curry2 and curry3 are in for now, without placeholders in their direct parameters yet. Those two are at least straightforward to write manually, especially with AwaitingTwo/Three interfaces already in place.

Logic is pretty much done. Shout if any of those look off. Missed the auto-currying. Had to check with some runtime tests. Going back for these too.

I'll push curryN a bit further, then I'm thinking Composition -> Object -> List -> Array.

@KiaraGrouwstra
Copy link

@miangraham: if I can finish my current PRs overloads may no longer be needed (curry PoC at microsoft/TypeScript#5453 (comment)).
I presume you already saw, but Ramda typings have a curryN with template here, and code-gen'd results for branches dist-simple, dist-placeholder, dist.

@miangraham
Copy link

@tycho01 Ooo, very nice. I'd seen Ramda's generation approach but didn't know about any of the TS work on variadic kinds. That looks like an awesome shortcut. Existing interfaces with multiple fixed arities would still need overloads but they could just be typed in terms of the variadic version, saving a ton of boilerplate. I think. arity(5)(curry), etc. Super cool.

@KiaraGrouwstra
Copy link

@miangraham: Yeah. Ideally type programming should be no worse than the expression level. Heck, in the ideal scenario the standard library would be well-typed enough for it to just infer these return types rather than us having to do them manually. Much less with clunky overload codegen.

@davidchambers
Copy link
Member Author

Thanks for the contributions, @miangraham. Keep them coming. :)

You may have an opinion on #438. If we decide to abandon Ramda-style currying our TypeScript type definitions will no longer be so unwieldy.

@miangraham
Copy link

I'm currently erring towards wide and shallow treatment to try and get an MVP/first pass done for the whole library. Means skimping on currying where it's not easy while leaving as many explicit TODOs as possible, but could result in getting something out fairly soon. I'm thinking this month if we're lucky?

Going breadth-first also enables fundamental changes in approach. If the TS defs really want to bite off whole-hog fancy currying, that probably means code generation, a separate repo to cordon off the infrastructure to generate the final .d.ts, more exhaustive testing, etc etc etc. Plenty of great Ramda examples to lean on there when it's needed.

In the meantime it'd be nice to get users just statically typing the basics to see how it goes!

@davidchambers
Copy link
Member Author

In the meantime it'd be nice to get users just statically typing the basics to see how it goes!

I agree. Let's focus on the curried signatures initially. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants