TypeScript support #431

davidchambers · 2017-07-30T16:54:20Z

🚧 👷‍♀️ 🚧

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 · 2017-07-30T17:10:13Z

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.

davidchambers · 2017-07-30T17:12:41Z

index.d.ts

+}
+interface Alternative<A> extends Applicative<A>, Plus<A> {
+  constructor: ApplicativeTypeRep<A> & PlusTypeRep<A>
+}


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?

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

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

KiaraGrouwstra · 2017-07-30T17:35:45Z

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! 😃

davidchambers · 2017-07-30T17:44:23Z

index.d.ts

+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;


Do you have a suggestion, @tycho01?

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.

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.

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.

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 · 2017-07-30T17:57:38Z

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.

Avaq · 2017-07-31T12:46:53Z

index.d.ts

+  (a: A, b: B, c: C): R
+  (a: A, b: B): (c: C) => R
+  (a: A): AwaitingTwo<B, C, R>
+}


I've seen these before somewhere. 🤔

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

ikatyang · 2017-07-31T13:01:53Z

index.d.ts

+
+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>>;


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.

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

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.

simple curry

template -> generated

various curry

template -> generated

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

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?

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?

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.

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.

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. :)

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

Avaq · 2017-08-02T11:20:18Z

index.d.ts

+//  Placeholder
+
+interface Placeholder {
+  '@@functional/placeholder': boolean


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!

true is not a type, though, is it?

true is a literal type.

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

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. :)

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}

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

miangraham · 2017-08-30T23:42:43Z

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 · 2017-08-31T15:14:43Z

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 · 2017-08-31T21:18:04Z

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 · 2017-08-31T21:22:20Z

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 · 2017-08-31T22:32:22Z

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 · 2017-09-02T02:49:17Z

KiaraGrouwstra · 2017-09-02T02:57:41Z

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

miangraham · 2017-09-02T08:37:09Z

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 · 2017-09-02T09:03:06Z

@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 · 2017-09-02T09:42:38Z

@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 · 2017-09-02T09:52:04Z

@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 · 2017-09-03T13:49:15Z

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 · 2017-09-04T09:45:11Z

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 · 2017-09-04T10:23:13Z

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. :)

davidchambers commented Jul 30, 2017

View reviewed changes

Avaq reviewed Jul 31, 2017

View reviewed changes

ikatyang reviewed Jul 31, 2017

View reviewed changes

Avaq mentioned this pull request Jul 31, 2017

Add a TypeScript definition file fluture-js/Fluture#129

Merged

46 tasks

Avaq reviewed Aug 2, 2017

View reviewed changes

kurtmilam mentioned this pull request Sep 2, 2017

readable code without Ramda-style currying #438

Closed

davidchambers mentioned this pull request Oct 2, 2017

reconsider TypeScript support #446

Open

davidchambers added 5 commits March 13, 2018 12:52

TypeScript support

cd70faa

replace ‘A’ with ‘any’ in type definitions for S.parseJson

0754c90

more type definitions for S.equals

a44dd10

more type definitions for comparison functions

77ac50a

add missing parameter names

dd8466f

davidchambers and others added 21 commits March 13, 2018 12:52

initial support for placeholders

5b0ee39

use ‘true’ rather than ‘boolean’ in Placeholder type definition

f81c7e6

Fix for error TS7006 when building with noImplicitAny.

d730717

more type definitions

fe869b8

Automated typechecking/linting/testing for TypeScript definitions.

d998b96

Makefile and tslint.json cleanup

0c38120

First pass TS defs for Function and Logic, plus many tests

be10bd0

TS defs: Curried versions of Logic functions

5a16c53

TS defs: Composition types, Function and Logic improvements

a11808a

List first pass: Unary currying only

3952f97

remove placeholder support

98241de

TS defs: Array first pass, List fixes

1706c97

replace Ramda-style currying with regular currying

10e34fb

introduce Fn[1-5] types to reduce noise

51f060d

filterM

cd39ed4

type definitions for remaining functions (not methods)

1f73801

add TypeScript translation of Mocha test suite

2f42fa8

uncomment assertions

d8db0ee

patch

2fa600f

another patch

6b4f52c

simplify definitions of polymorphic functions

a543011

davidchambers force-pushed the davidchambers/typescript branch from e732768 to a543011 Compare March 13, 2018 12:00

remove tests for functions which no longer exist

e91ae97

davidchambers added the typescript label Nov 19, 2019

TypeScript support #431

Are you sure you want to change the base?

TypeScript support #431

Conversation

davidchambers commented Jul 30, 2017

davidchambers commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiaraGrouwstra commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miangraham commented Aug 30, 2017 • edited Loading

davidchambers commented Aug 31, 2017

miangraham commented Aug 31, 2017

davidchambers commented Aug 31, 2017

miangraham commented Aug 31, 2017 • edited Loading

miangraham commented Sep 2, 2017 • edited Loading

KiaraGrouwstra commented Sep 2, 2017

miangraham commented Sep 2, 2017 • edited Loading

KiaraGrouwstra commented Sep 2, 2017

miangraham commented Sep 2, 2017

KiaraGrouwstra commented Sep 2, 2017

davidchambers commented Sep 3, 2017

miangraham commented Sep 4, 2017

davidchambers commented Sep 4, 2017

Avaq Aug 2, 2017 •

edited

Loading

miangraham commented Aug 30, 2017 •

edited

Loading

miangraham commented Aug 31, 2017 •

edited

Loading

miangraham commented Sep 2, 2017 •

edited

Loading

miangraham commented Sep 2, 2017 •

edited

Loading