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

Why path isn't curried? #185

Open
goodmind opened this issue Jul 26, 2017 · 28 comments
Open

Why path isn't curried? #185

goodmind opened this issue Jul 26, 2017 · 28 comments

Comments

@goodmind
Copy link

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

@KiaraGrouwstra
Copy link
Member

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
Copy link
Author

@tycho01 can you do codegen now?

@KiaraGrouwstra
Copy link
Member

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

@ikatyang
Copy link
Member

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
Copy link
Member

Hm. Isn't dropping the overloads a regression?

@ikatyang
Copy link
Member

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

@KiaraGrouwstra
Copy link
Member

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

@goodmind
Copy link
Author

goodmind commented Aug 16, 2017

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

@ikatyang
Copy link
Member

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
Copy link
Author

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

@KiaraGrouwstra
Copy link
Member

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
Copy link
Author

goodmind commented Aug 16, 2017

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

@KiaraGrouwstra
Copy link
Member

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
Copy link
Member

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
Copy link
Member

KiaraGrouwstra commented Aug 16, 2017

Oh, sorry, yes, in 2.5 microsoft/TypeScript#17455 made the issue pop up everywhere, my recent PR should help fix that.

@goodmind
Copy link
Author

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

@KiaraGrouwstra
Copy link
Member

Right, that issue was only in some 2.5 versions.

@goodmind
Copy link
Author

goodmind commented Aug 16, 2017

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

@KiaraGrouwstra
Copy link
Member

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
Copy link
Author

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

@goodmind
Copy link
Author

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

@ikatyang
Copy link
Member

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

  • Obj is used to be Dictionary's alias, it is now only called Dictionary for consistency.
  • the new types are generic-dependency-sensitive
    • if there is a function prototype: <T, U>(a: T, b: U) => T | U
      • it might used to be something like:
        • <T, U>(a: T, b: U) => T | U
        • <T, U>(a: T) => (b: U) => T | U
      • but the generics are now all stick with its parameter
        • <T, U>(a: T, b: U) => T | U
        • <T>(a: T) => <U>(b: U) => T | U
    • if there is a function prototype: <T>(a: any, b: any) => T
      • it might used to be something like:
        • <T>(a: any, b: any) => T
        • <T>(a: any) => (b: any) => T
      • but the generics are now all stick with the last function, suppose the last is the most specific
        • <T>(a: any, b: any) => T
        • (a: any) => <T>(b: any) => T
    • if there is a various function prototype: map for example
      • if you only specify the first parameter:
        • the old types considered it as the last kind: one of list, functor, object.
        • the new types considered it as their mixed/general kind (list + functor + object) unless you use selectable-overload to specify which kind to use: R.map<'1', 'list'>() ('1' means selected the 1-param version, list means selected the list version)
  • the new types prefer type assertion explicitly for those simple generic, for example:
    • the old types
      • messagesPath<Store['messages']['byId']>(state)
      • you have to mouse hover to the variable to see what the return type it is if you're not familiar to this type
    • the new types prefer
      • (messagesPath(state) as Store['messages']['byId'])
      • you can obviously know what the return type will be

@KiaraGrouwstra
Copy link
Member

@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
Copy link
Member

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

@KiaraGrouwstra
Copy link
Member

Fair enough. :)

@KiaraGrouwstra
Copy link
Member

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

@goodmind
Copy link
Author

goodmind commented Aug 24, 2017

@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
Copy link
Member

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

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

No branches or pull requests

3 participants