[ramda] remove placeholder

[adispring]

Ramda does not support TS well, one of the reason is TS does not support curry well enough, and R.placeholder make this worse, such as put a function with R.placeholder as a param into another function, in this case TS can not infer the type correct.

R.merge and R.contains have been removed since v0.28.0.

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

Select one of these and delete the others:

If adding a new definition:

• The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
• If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
• Create it with dts-gen --dt, not by basing it on an existing project.
• Represents shape of module/library correctly
• tslint.json should contain { "extends": "@definitelytyped/dtslint/dt.json" }, and no additional rules.
• tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

If removing a declaration:

• If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
• Delete the package's directory.
• Add it to notNeededPackages.json.

[typescript-bot]

@adispring Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• ramda — on npm, on unpkg (author is owner)

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

• ❌ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Most recent commit is approved by type definition owners or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!

---

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 59579,
  "author": "adispring",
  "headCommitOid": "39117534e75ebe920bc1baa8b43b383e565a7ef1",
  "mergeBaseOid": "863269c22f56d3491788a68cb70ac23ce78f3f7d",
  "lastPushDate": "2022-04-08T01:09:40.000Z",
  "lastActivityDate": "2022-04-18T05:31:40.000Z",
  "hasMergeConflict": true,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "ramda",
      "kind": "edit",
      "files": [
        {
          "path": "types/ramda/README.md",
          "kind": "markdown"
        },
        {
          "path": "types/ramda/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/ramda/test/assoc-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/assocPath-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/concat-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/contains-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/curry-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/curryN-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/divide-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/equals-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/flip-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/groupWith-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/gt-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/gte-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/has-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/lt-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/lte-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/mathMod-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/merge-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/modulo-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/partition-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/prop-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/propOr-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/test/subtract-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/ramda/tools.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "TheHandsomeCoder",
        "donnut",
        "mdekrey",
        "sbking",
        "afharo",
        "teves-castro",
        "hojberg",
        "samsonkeung",
        "angeloocana",
        "raynerd",
        "moshensky",
        "ethanresnick",
        "deftomat",
        "blimusiek",
        "biern",
        "rayhaneh",
        "rgm",
        "drewwyatt",
        "jottenlips",
        "minitesh",
        "krantisinh",
        "nemo108",
        "jituanlin",
        "Philippe-mills",
        "Saul-Mirone",
        "Nicholaiii",
        "LORDBABUINO",
        "couzic",
        "NEWESTERS",
        "adispring",
        "essenmitsosse"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "rbuckton",
      "date": "2022-04-07T16:41:26.000Z",
      "abbrOid": "768bbc5"
    },
    {
      "type": "stale",
      "reviewer": "essenmitsosse",
      "date": "2022-03-31T14:19:37.000Z",
      "abbrOid": "768bbc5"
    }
  ],
  "mainBotCommentID": 1082591447,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @TheHandsomeCoder @donnut @mdekrey @sbking @afharo @teves-castro @hojberg @samsonkeung @angeloocana @raynerd @moshensky @ethanresnick @deftomat @blimusiek @biern @rayhaneh @rgm @drewwyatt @jottenlips @minitesh @Krantisinh @Nemo108 @jituanlin @Philippe-mills @Saul-Mirone @Nicholaiii @LORDBABUINO @couzic @NEWESTERS @essenmitsosse — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[DangerBotOSS]

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

ramda (unpkg)

was missing the following properties:

1. collectBy
2. modify
3. modifyPath
4. partialObject
5. on

as well as these 5 other properties...

promap, sequence, splitWhenever, unwind, whereAny

Generated by 🚫 dangerJS against 3911753

[essenmitsosse]

I can't really speak to the removal of Placeholder since I do not use it mayself for the reasons you stated und because I find the handling just weird anyway. Don't know what the implications are for other people.

So from my side, this is good to go.

[typescript-bot]

@adispring Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

[typescript-bot]

@essenmitsosse Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[ycmjason]

R.placeholder make this worse, such as put a function with R.placeholder as a param into another function, in this case TS can not infer the type correct.

Can you give an example how the overloads with placeholder will break the type inference?

[somebody1234]

(And has it been confirmed that moving placeholder overloads below normal overloads will not fix this?)

[ycmjason]

((will removing the placeholder overloads be a breaking change for code that is currently using them?))

[adispring]

R.placeholder make this worse, such as put a function with R.placeholder as a param into another function, in this case TS can not infer the type correct.

Can you give an example how the overloads with placeholder will break the type inference?

@ycmjason Only few ramda apis' types have Placeholder overloads, and these Placeholder overloads can not support full ability of ramda's Placeholder.

Even worse, some api's Placeholder overload implementation is wrong.

Ramda and Typescript are just tools help people do their jobs more efficiently and safely, not the opposite way.

For example:

R.someApi(R.__)(R.__)(R.__)(...) is valid, we can write any number of R.__s if the api has not enough arguments to execute. but we can't do that in Typescript with ramda.

Here are some more examples:

1. R.assoc

R.assoc(R.__); // Argument of type 'Placeholder' is not assignable to parameter of type 'string'.ts(2345)
R.assoc(R.__, 'value'); // Argument of type 'Placeholder' is not assignable to parameter of type 'string'.ts(2345)

2. R.assocPath, R.prop, R.propOr .etc, have the same problem.

3. R.concat

R.concat(R.__)('abc'); // Expected 2 arguments, but got 1.ts(2554)

4. R.includes

In R.includes, the placeholder overload implementations are wrong:

export function includes(__: Placeholder): (list: readonly string[] | string, s: string) => boolean;
export function includes<T>(__: Placeholder): (list: readonly T[], target: T) => boolean;

should be:

export function includes(__: Placeholder): (s: string, list: readonly string[] | string) => boolean;
export function includes<T>(__: Placeholder): (target: T, list: readonly T[]) => boolean;

[adispring]

Ramda itself may also want to remove Placeholder in the future, here are some discussions:

ramda/ramda#2705

ramda/ramda#2629

[essenmitsosse]

I gave it some more thought and I agree with @adispring.

I think the philosophy here should be: If you can't get it right, better leave it out. People use TypeScript to get more confidence in their code — the worst you want is a false sense of security or a feature that randomly works in some cases and in some it doesn't. It's a much clearer statement for a user to just know Placeholder is not supported, then guessing or even having to dive into the types themselves to figure out when it is and when it isn't.

I'd rather have this library focus on the things it can reasonably support and do those things well. Also everybody is free to fork these types and add placeholder support and release a ramda-types-with-partial-placeholder-support version of this library. But that also reflects my use case, so I'd love to hear other peoples opinions.

Additionally I think I read on the ramda repo the other day that the future of Placeholder is unclear anyway — but not 100% sure about that and can't find it anymore. But maybe @CrossEye could chime in and give his opinion on the matter.

[somebody1234]

TL;DR: these aren't very convincing reasons to remove placeholder - so at the very least we should get the opinion of the community first.

sourcegraph results may be useful to see how widely used placeholder is:
8.7k results for ramda, 274 for ramda AND R.__, which works out to a little over 3% of repositories that use placeholder
in js, 1.2k out of 24.4k = ~5%, if conversion from js is a goal.
github says 15k out of 323k = ~5% as well, however this search may be a bit more fuzzy than desired.

anyway re: placeholder overloads - i don't think they're particularly difficult to add, so i strongly disagree with using it as a reason they should be removed.

vaguely related - the types do not fully support ramda's api even without placeholder, since we do not support () currying which a) is supported by ramda and b) is supported by an old set of typings for specifying type parameters. so does that mean we should remove all typings, since none of them are currently typed correctly?

as for wrong overload implementations, this is not unique to placeholder either. so again, it should not be a reason to remove placeholder.

[somebody1234]

actual tl;dr: some typings, is better than no typings (alternatively, not breaking things is better than breaking things) - so without a good reason, i would prefer not removing them

[adispring]

TypeScript is just a static tools, I don't think this breaking change will introduce much destructive effect, it will effect nothing in run time.

People can easily replace R.__ with lambda express by ts compiler check tips, if he/she is using ramda with ts.

[adispring]

274 for ramda AND R.__, which works out to a little over 3% of repositories that use placeholder
in js

I've just seen these 274 use case of R.__:

1. most of them are Ramda DT type unit-test;
2. some of them are not even real R.__ they are just some property start with __ in a object named r or R;
3. some are real use of R.__, but unfortunately, people have to use @ts-ignore above the line of R.__ to ignore ts compiler‘s grumble;

[typescript-bot]

@adispring The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

[typescript-bot]

@adispring The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

[CrossEye]

@essenmitsosse:

Additionally I think I read on the ramda repo the other day that the future of Placeholder is unclear anyway — but not 100% sure about that and can't find it anymore. But maybe @CrossEye could chime in and give his opinion on the matter.

There are no immediate plans for removing the placeholder. There is a certain distaste for it among the core team, and I think many of us would be happy to see it go. But there is no obvious replacement for this without a radical change to the library. That may happen one day, but it's not in the plans for 1.0 or 2.0.

I could see us disallowing the placeholder being passed as the last argument in any one call. It makes no logical sense, and probably contributes to the difficulties typing functions here. It's allowed because there was no reason to add complexity to the code in order to disallow it; and if it's allowed, there is no reasonable alternative but to treat it as a no-op. But if this is a significant factor in making it hard to type Ramda, we could definitely at least fix that.

I really don't know TS well, and am not versed in the details of the typings files. But would disallowing that make typing Ramda's more complex signatures more palatable? Ramda has always been a JS library, and we haven't really even tried making concessions in order to work better with TS. We probably still wouldn't do so for our core API design, but this is a minor implementation detail. It would technically be a backward incompatibility, but as we (finally) approach 1.0, we're going to have a few of those, so this might be a good time to talk about it.

We could also do something to the actual value, if it helps. It's an object with a special named property on it; that's it. If it would help for it to be of some constructed type, we could probably do so, so long as we can keep that property.

[typescript-bot]

@rbuckton, @essenmitsosse Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[essenmitsosse]

@CrossEye thanks for the input, that is definitely helpful.

Regarding disallowing certain use cases for Placeholders in the JavaScript implementation: It say it doesn't really matter for the Typings. If we got the input that certain use cases aren't really intended or necessary, its perfectly fine to not support them in TypeScript, while they still might be valid when actually running the code.

Example:

const add = (a, b) => a + b

Assuming this function is supposed to add numbers, its implementation would allow it to also concatenate strings. So when writing a JavaScript library you might want to add a run time check to avoid someone foot-gunning themself. But in TypeScript, you could type this function as only being allowed to take a number — making it actually easier to narrow the use case, without the need for runtime checks.

To me, that sounds like your "There is no point in allowing R.__ as a last argument"-example — if thats the intention of the library: Just don't add an overload for it.

---

What makes Placeholders so tricky to type is: You need to either
a) type each use case by hand
b) have some quite complex generic typing to automatically generate the overloads.

a) will inevitably miss some use case and will still be quite a lot of work. It will also be super hard to maintain.

b) seems to be the easier approach, but it will sooner or later reach its limitation — at the latest when generics come into play (probably making you have to fallback on a)). It most likely will also make the resulting types super hard to read — probably for everybody, even people who never want to use Placeholders.

---

My assessment would be that adding full Placeholder support would most definitely be fantastic for people who actually use Placeholders and TypeScript — if we ever get it to fully work and not fail halfway through. But it will be an immense amount of work on the whole library (just for one feature) and make maintaining much harder in the future.

For me this brings us back to the question: is it really worth it? All the work put into supporting Placeholders, will not only not at all benefit people who don't use. It will probably make the library more error prone and less well maintained and up to date, then if we just dropped support.

---

I agree with @rbuckton, that we should look at the ramifications, of dropping support for Placeholder. But we should also look at the ramifications of continuing support for it or even expanding it. Right now it looks like, Placeholder is of little use, when used with TypeScript due to how little support there is as of now.

Maybe someone could show some examples of how function types would change with full Placeholder support?

[CrossEye]

Maybe someone could show some examples of how function types would change with full Placeholder support?

I can't do that; I don't have the necessary TS skills. But it will multiply the number of signatures greatly, even if Ramda disallows a final placeholder argument. If it doesn't, I'm not sure what can be done, as this is perfectly legal in Ramda:

foo (arg1) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (__) (arg2)
//         `--------------------------------------- n times ----------------------------------------'

for any non-negative integer, n.

So if foo is a Ramda-curried version of (a, b, c) -> d, it supports any of these calls:

foo (a, b, c)
foo (a, b) (c)
foo (a) (b, c)
foo (a) (b) (c)
foo (__, b) (a, c)
foo (__, b) (a) (c)
foo (__, b) (__, c) (a)
foo (__, b, c) (a)
foo (__, __, c) (a, b)
foo (__, __, c) (a) (b)
foo (__, __, c) (__, b) (a)
foo (a, __, c) (b)

(and I'm likely missing some.)

This does not include the stupid calls such as f (a) () () (b) () () () () (c), which I think we can safely ignore. Anyone doing that deserves to get a hand slapped.

Am I correct that even if a signature is not specified in the typings file, the user can overwrite the type of any given expression as needed? So if the Ramda-supported type was not supplied by TS, the user could still describe the type manually?

[essenmitsosse]

I actually played around a little bit, and it seems with recursive types implementing Placeholder would actually be possible — including all the weird edge cases.

Playground

Downsides are:

• Types have to be written by hand (at least partly, there might be some abstractions) — which would lead to an explosion of complexity
• The resulting types work very well. Actually exactly as expected. But they are pretty unreadable to the user, since they are obfuscated behind an interface (I don't know how to make the overloads visible, while still making them recursive)

[essenmitsosse]

If we wouldn't have to worry about generics you could type it like that, which would result in quite decent types:

Playground

But there are barely Ramda functions that wouldn't need generics. :/

[somebody1234]

Worth noting that while it is legal - is it a case that Ramda wants to support?
I feel like if your code looks like that, usually it means something has gone terribly wrong at some point.
(So I'd put it in the same bucket as said "stupid calls" - however of course it's a completely separate issue whether recursive interfaces (which incidentally, would allow calls such as that) would be a net positive)

As for overwriting the type - yes, I think it is possible:
Playground

Not to mention there is always the type assertion as the last resort - something like:

const add: {
  (my: Overload): 1;
  (my: Overload): 2;
} = R.add as any;

[typescript-bot]

@adispring Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

[CrossEye]

Worth noting that while it is legal - is it a case that Ramda wants to support?

We could change this behavior, while it is intentional, it is mostly just there for logical compatibility and not for practical work. I would also have no objection to having TS reject it even if underneath Ramda still allows it.

The reasoning is simple enough for empty argument lists returning a function equivalent to the one that was called. If we start with, say, four formal parameters, and we supply four of them, we're done and we call the function. If we supply three of them, we get back a function expecting the remaining one. If we supply two, we get back a function expecting the remaining two. If we supply one, we get back a function expecting the remaining three. So it seems eminently logical that if we supply zero, we get back a function expecting all four.

For placeholder, it's a very similar case. Again, with four formal parameters, if we supply non-placeholder values at indices 0 and 3, we get back a function that will fill indices 1 and 2. If we supply non-placeholder values at index 2, we get back a function that will fill indices 0, 1, and 3. If we supply non-placeholder values at index 3, we get back a function that will fill indices 0, 1, and 2. So if we supply no non-placeholder values, we get back a function that will fill indices 0, 1, 2, and 3.

This feels much like the fact that [] .every (anyPredicate) //=> true or sum ([]) //=> 0. It's logically consistent and occasionally useful.

The big problem for Ramda if we were to change this is that our only alternative here would likely be to throw an exception, and Ramda, whose main focus is on composability, is very loathe to throw. But we can work that out. I wouldn't bother, though, if this is something that TS can handle without crazy gymnastics.

[somebody1234]

TL;DR - sorry, bad wording - I meant more along the lines of "is this an intended usecase of Ramda" rather than "maybe Ramda should go out of its way to disallow it". Not specialcasing it in the implementation definitely makes things a lot easier to reason about, among other things

IMO it's less about "the types should reflect the semantics of Ramda 1:1" and more "the types should reflect Ramda's intended usecases" -
In situations like these it does feel like doing certain things R.foo()()()(), R.foo(__)(__)(__)(__) etc. are probably almost always a mistake

Or in other words, the way I see it, TypeScript's intention is not to be a type system so much as it is meant to be a tool to help in writing correct code

[somebody1234]

Note though, I think to support placeholder well, we'd need to add a huge number of new interfaces (for all possible combinations of placeholders and filled-in parameters) - I'm not sure if it's possible to make a helper type but I wouldn't be optimistic because said helper type would have to work correctly with generics

So the question is, is the relatively low fraction (not sure how to estimate actually) of users using placeholder, worth making the types several times bigger (and potentially less ergonomic)

[CrossEye]

Is there some well-respected introduction to TS typings files?

I think I'm still missing a lot of the big picture. I'm not sure how much time and energy I want to invest in learning about them, but if I do choose to put some real effort into it, what's a good place to start? I've seen plenty of the TS for JS developers articles and the I never thought I'd switch, but now I'm so happy ones, and none have really struck any chords for me. If I'm going to dig in, it's probably from this angle. How can we handle JS's trickier signatures?

[somebody1234]

I think the handbook is what's generally recommended - however note that the other sections (on TypeScript in general) are probably useful too - for the most part this is TypeScript definitions, anything specific to typings files that may have needed to be done has already been done.

Supposedly it takes 30 minutes to go through, although I suspect it'd take quite a bit longer to absorb - and even longer still to play around with specific typings(/constructs) to get a feel of the strengths and weaknesses of each

[typescript-bot]

@adispring I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on May 18th (in a week) if the issues aren't addressed.

[typescript-bot]

@adispring To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!