typed-typings / npm-ramda

TypeScript's type definitions for Ramda
MIT License
384 stars 64 forks source link

Why path isn't curried? #185

Open goodmind opened 7 years ago

goodmind commented 7 years ago

Why path isn't curried? (I mean ones with mapped types). And why T was replaced with T | undefined in last update?

KiaraGrouwstra commented 7 years ago

Path... I wish we could properly type it yet. :)

Why path isn't curried? (I mean ones with mapped types).

tl;dr: just hadn't bothered to do the curried codegen for those variants. it sucks a bit worse than you'd expect as the versions from different typing strategies must be mixed, lest TS ignore all but the first option with matching first param set.

T | undefined... I sorta get where you're coming from. So technically, if a path fails, Ramda does return undefined at run-time. The idea was that if you did supply the second arg right away, it could verify the return type, while otherwise it'd be... not necessarily guaranteed to work.

I think using the assertion operator ! you'd be able to scrub this | undefined off again so as to defer potential issues back to run-time if you're confident it will normally work. Might that suffice for you?

goodmind commented 7 years ago

@tycho01 can you do codegen now?

KiaraGrouwstra commented 7 years ago

@goodmind: @ikatyang would know more about the new codegen. The iteration-based alternative, not yet.

ikatyang commented 7 years ago

The codegen is always available since it's just auto-currying, but the question is, is its prototype (*.d.ts in templates/ folder) curry-able? Its prototype currently is not curry-able, so now it just uses the general template and mixin with the old version, I'm still waited for its type-level operation version since it'll be curry-able.

KiaraGrouwstra commented 7 years ago

Hm. Isn't dropping the overloads a regression?

ikatyang commented 7 years ago

The overloads are mixin with the template, so that it isn't dropped. See dist branch/src/path.d.ts

KiaraGrouwstra commented 7 years ago

@goodmind has @ikatyang's recent refactor addressed this for you?

goodmind commented 7 years ago

@tycho01 well, I can't even update to new types because there's dozen of errors

ikatyang commented 7 years ago

New types removed some outdated types (R.isArrayLike, R.isNaN, etc., they're not exist in v0.24), and some types' generics are changed (some of them does not need to use type parameter explicitly now).

@goodmind Is the codebase public? If you need help, I can look into them to find out what the problem is.

goodmind commented 7 years ago

@ikatyang yes, here is pull https://github.com/goodmind/treact/pull/44 and here is log of ts errors https://travis-ci.org/goodmind/treact/jobs/265130394

KiaraGrouwstra commented 7 years ago

NumberToString errors look familiar to me; I only just sent a PR to TS for that two days ago. That makes me wonder how they were already used in typings here without exploding during the tests. I'm also seeing a couple placeholder signatures by the way; I wonder if the simple distribution might fare a bit better (fingers crossed). But yeah, thanks for submitting the log, that's pretty valuable.

goodmind commented 7 years ago

@tycho01 well, I'm using nightly typescript, maybe this an issue?

KiaraGrouwstra commented 7 years ago

In my typical lib where I'm testing types like that NumberToString this TS PR resolved 90% of the errors I got of that type, so if your nightly is new, it should help rather than hurt. I'm testing that with master as well at this point, so with that the nightlies should be better in sync, if anything. Looks like the Ramda typings themselves are currently tested with stable though.

ikatyang commented 7 years ago

NumberToString[N] works without errors for me (and tsc --skipLibCheck false passed on travis) with TS v2.4.2, I'm not sure what causes this error here, maybe I should turn off them until TS 2.5 release.

KiaraGrouwstra commented 7 years ago

Oh, sorry, yes, in 2.5 https://github.com/Microsoft/TypeScript/pull/17455 made the issue pop up everywhere, my recent PR should help fix that.

goodmind commented 7 years ago

@tycho01 i tested against 2.4.2 and there is no NumberToString errors

KiaraGrouwstra commented 7 years ago

Right, that issue was only in some 2.5 versions.

goodmind commented 7 years ago

@tycho01 Also I wonder why you returned to CurriedFunctionN interface?

KiaraGrouwstra commented 7 years ago

I guess what you're saying is the pre-curried functions were better for having parameter names? Or are you getting inference issues related to these?

goodmind commented 7 years ago

@tycho01 yes, there's something with inference, because it worked with old types (maybe old types was wrong who knows)

goodmind commented 7 years ago

Here's log with typescript@2.4.2 https://travis-ci.org/goodmind/treact/jobs/265147375

ikatyang commented 7 years ago

While looking into it, I found the problems are mostly the following things:

KiaraGrouwstra commented 7 years ago

@ikatyang that looks like a relatively good upgrade guide. perhaps we could put it in the readme or a changelog or something. hopefully that would help save a few questions.

ikatyang commented 7 years ago

@tycho01 maybe we could open an new issue for upgrade guide just like ramda does?

KiaraGrouwstra commented 7 years ago

Fair enough. :)

KiaraGrouwstra commented 7 years ago

@goodmind: has @ikatyang's feedback helped resolve your issues here?

goodmind commented 7 years ago

@tycho01 well, I tried to update but all this selectable-overloads looks too much verbose, but without them it can't infer type correctly

KiaraGrouwstra commented 7 years ago

@goodmind: I'd prefer if it could automatically get things right as well, I would say that is the ideal goal here