@jakebailey Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

• gensync (new!) — on npm, on unpkg
  • 2 added owners: ✎@jakebailey, @nicolo-ribaudo
  • Config files to check:
    • gensync/tslint.json: not the expected form (check: rules)

Code Reviews

This PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged.

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

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• 🕐 Only a DT maintainer can approve changes when there are new packages added

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

{
  "type": "info",
  "now": "-",
  "pr_number": 60125,
  "author": "jakebailey",
  "headCommitOid": "cbd3f0c15f53a12ce0728e39898668c6024642e5",
  "mergeBaseOid": "922d794d523ab3926fa6a65466e73bbf8b3b8598",
  "lastPushDate": "2022-07-06T17:14:23.000Z",
  "lastActivityDate": "2022-07-08T17:20:43.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "gensync",
      "kind": "add",
      "files": [
        {
          "path": "types/gensync/gensync-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/gensync/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/gensync/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/gensync/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `rules`)"
        }
      ],
      "owners": [],
      "addedOwners": [
        "jakebailey",
        "nicolo-ribaudo"
      ],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "peterblazejewicz",
      "date": "2022-05-01T19:55:23.000Z",
      "abbrOid": "230423e"
    }
  ],
  "mainBotCommentID": 1113604040,
  "ciResult": "pass"
}

🔔 @jakebailey — you're the only owner, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...)

@zxbodya @loganfsmyth @nicolo-ribaudo maybe have opinions (sorry for the ping); I did this out of interest for the library after I saw it mentioned on one of prettier's GitHub issues, so I have no major attachment other than thinking this library was compelling and was surprised to find it not on DT.

@peterblazejewicz 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?

Thanks for the ping! I'll test these types in our codebase, and check if there is any problem.

I would treat this as a popular/critical package (even though the labels don't indicate it), gensync gets some 21 million weekly downloads.

Almost 100% of those weekly downloads come from @babel/core, and gensync is just an internal implementation detail (not re-exposed to other Babel types).

Almost 100% of those weekly downloads come from @babel/core, and gensync is just an internal implementation detail (not re-exposed to other Babel types).

I was mainly worried about people installing @types/gensync, then those types being applied within babel and then breaking the API via some chain of exports. Maybe that's not a thing that'd happen thanks to d.ts files locking types in.

Let me know how they go; the type variables are different, so it won't be a simple swap; I was going to test this myself, but I had already been nerd sniped by typing this library enough and needed to go back to work. 😅

I'm testing these definitions in Babel's source. Our approach of passing a (...: Params) => Return function as the type parameter instead of a Params tuple and a separate Return parameter feels nicer, but I could get used to it.

Also, could you verify if this code type-checks properly with your type definitions? I'm getting an error about true not being compatible with never, but it could be an error in my adaptation of your definitions to our style.

const isAsync = gensync<[], boolean>({
  sync: () => false,
  errback: cb => cb(null, true),
});

declare module "gensync" {
  import gensync_dt = require("gensync_dt");

  export type Gensync<Fn extends (...args: any) => any> = gensync_dt.Gensync<
    Parameters<Fn>,
    ReturnType<Fn>,
    any
  >;
  export type Handler<R> = gensync_dt.Handler<R>;
  export type Options<Fn extends (...args: any) => any> = gensync_dt.Options<
    Parameters<Fn>,
    ReturnType<Fn>,
    any
  >;

  declare const gensync: {
    <Fn extends (...args: any) => any>(
      generatorFnOrOptions:
        | ((
            ...args: Parameters<Fn>
          ) => Generator<gensync_dt.Handler, ReturnType<Fn>>)
        | Options<Fn>,
    ): Gensync<Fn>;

    all<Return>(gensyncs: Array<Handler<Return>>): Handler<Return[]>;
    race<Return>(gensyncs: Array<Handler<Return>>): Handler<Return>;
  };
  export default gensync;
}

I tried debugging the isAsync error a bit, and it's failing for the same reason that a(true) fails in this example:
https://www.typescriptlang.org/play?ts=4.6.2#code/C4TwDgpgBAwghgGwQIzgYwNYB4BKA+KAXihyggA9gIA7AEwGcoA3AewEtaoB+KACggBOAgFxQ41EAEoiBVhyij+Q0eJAAaKAIj0ArgmCic0wrPa0A3AChLtCGgRwtzR2NHwkqTFmQsWCCOJ4VpZwvNR6CBrAAjoQklZAA

type Callback<R, E = unknown> = R extends void ? (err: E) => void : (err: E, result: R) => void;

declare var a: Callback<boolean>;

a(null, true);

However, I don't know how to fix it.

EDIT: Rewriting Callback like this seems to fix it:

  type Callback<R, E = unknown> = (
    ...args: R extends void ? [err: E] : [err: E, result: R]
  ) => void;

EDIT 2: Probably it should use E | null:

  type Callback<R, E = unknown> = (
    ...args: R extends void ? [err: E | null] : [err: E | null, result: R]
  ) => void;

I'm testing these definitions in Babel's source. Our approach of passing a (...: Params) => Return function as the type parameter instead of a Params tuple and a separate Return parameter feels nicer, but I could get used to it.

The main reason I wrote it this way is that it's generally preferred to not put all of the constraints into type variables and then have to pick the info back out again with utility types. I would expect that nearly all of the time, these functions are correctly typed via inference (so the type var detail doesn't matter so much); do you see it another way?

EDIT: Rewriting Callback like this seems to fix it:

That's weird; swapping it out for that tuple-based definition of Callback does fix the type error, but I'm not sure why it'd matter; it looks worse in tooltips (says Callback<...> and not (...) => void so I'll want to see if there's a better fix for that one.

EDIT 2: Probably it should use E | null:

RE: | null, it's a little unclear if that's a good idea or not; my intent was to not refer to null because when you hand gensync something like fs.readFile, that callback already includes the | null, and some libraries may not.

Confusingly, gensync actually calls the function with undefined in some cases, implying that maybe your callback can get undefined, but then checks for null in others, which implies that it also needs null. Maybe it's a bug, but the lib hasn't been updated in a while. EDIT: I also overlooked that it was == not ===, so either null or undefined are valid in that one code path, but the library will still "call" users of errback with an unexpected undefined.

I'll have a chance to look at this tomorrow.

You (I think) also had a code snippet like this in one of your comments:

const runGenerator = gensync<[item: Generator], any>(function* <Return>(item: Generator<unknown, Return>) {
    return yield* item;
});

What happened to that? Did you find a workaround? The other goal I had with the types was for the generators to be "closed", in that you shouldn't yield from generators not produced by gensync, under the assumption that doing so would be unsafe because the Symbols gensync uses as sentinel values are not exposed and therefore not possible to emulate. Did you have a working code sample that yielded an arbitrary generator and it worked?

Actually, while scrolling through the tweets to find the explanation for the type var thing, I found the answer for the conditional type thing (probably; combined with the whole boolean = true | false thing); I'll do more reading and issue searching to figure out if there's some other way to do what intended.

Also: https://stackoverflow.com/questions/61926729/why-does-a-typescript-type-conditional-on-t-extends-undefined-with-t-instanti

Fixed easily enough via the fix on microsoft/TypeScript#37279 (comment).

The runGenerator was just because I unknown is stricter than any, I realized it was a "good error" right after posting it so I deleted it.

The main reason I wrote it this way is that it's generally preferred to not put all of the constraints into type variables and then have to pick the info back out again with utility types. I would expect that nearly all of the time, these functions are correctly typed via inference (so the type var detail doesn't matter so much); do you see it another way?

Ok, that makes sense. For some reason we explicitly pass all the type parameters, I'll check.

I can absolutely see you writing the types explicitly to encode the API, though I think babel's current types don't allow for encoding of the errback error type (very possible nobody actually consumes those when there are sync and async versions available).

I think I could equivalently make the first type parameter be the sync function type, and keep a defaulted E parameter for that, though.

I don't have massively strong feelings either besides my general feeling towards keeping type params simple. I'm already sort of drive-by causing breaks for you as it is!

What do folks want to do with this PR?

I'd still like feedback from @nicolo-ribaudo whenever he gets a chance. I could just convert this into a draft to get this out of the maintainer queue for now.

I can imagine switching to something like this, if my type var differences are too problematic:

declare function gensync<Fn extends (...: any[]) => any, E = unknown>(...)

Which will be compatible with babel's existing types (and then also provide an error type), but Parameters and ReturnType will have to be inserted everywhere.

@nicolo-ribaudo I've added you as an owner; I can't seem to get the inference to work, so if you're happy with these written less-functionally, then I think they are where they need to be.

Were you able to see how this works in babel, if inference of functions works rather than explicitly specifying the signatures everywhere?

Re-ping @«anyone?»:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

@jakebailey 🤷‍♂️merge?

Whops sorry, I'm testing these right now in the Babel repo

@jakebailey It looks like inference never works for the return type:

import gensync, { type Handler } from "gensync";

const transform = gensync(function*(param: string): Handler<number> {
  return 2;
})

// Error: Type 'unknown' is not assignable to type 'number'
const num: number = transform.sync("foo");

Oh well, I don't know why but it's working now for me.

It's working well! babel/babel#14710
It would be nice if you could export the Callback type, because it's incompatible with a naive (err: unknown, result: R) => void type (because of the conditional).

I'm out of office, but I'll come back to this when I am back next week.

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @jakebailey.

(Ping @«anyone?».)

I'll export the type, but can you provide an example of what you were trying to do? I'm curious if there's a better way for these types to work if the conditional is causing problems.

Sure! I'm using it in https://github.com/nicolo-ribaudo/babel/blob/8bec7d53e6156f755c983ee1b75f362ee574474c/packages/babel-core/src/config/index.ts#L52-L54 and https://github.com/nicolo-ribaudo/babel/blob/8bec7d53e6156f755c983ee1b75f362ee574474c/packages/babel-core/src/gensync-utils/async.ts#L12-L18.

Ah, I saw those, I was moreso trying to understand

it's incompatible with a naive (err: unknown, result: R) => void type (because of the conditional)

I think I was trying to define my Callback<R> type just as (err: unknown, result: R) => void; because I knew that R is never void, but I now see that it's obvious that it doesn't work (I would need something like type Callback<R extends not void> = (err: unknown, result: R) => void.

But the reason it is good to expose the Callback type is exactly that otherwise users (aka I) always have to write the full longer type and cannot use a simpler-but-compatible-for-them type.

The PR has Callback exported now, I do think it's a good idea to export it and I probably just forgot when I was moving code around (not so good to have an exported type depend on an unexported one).

If everything looks solid with this PR, I can click the button.

Looks good to me!