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

feat: Add regenerator‑runtime type definition #46276

Merged

Conversation

ExE-Boss
Copy link
Contributor

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. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

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 be present and it shouldn't have any additional or disabling of rules. Just content as { "extends": "dtslint/dt.json" }. If for reason the some rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

@typescript-bot typescript-bot added New Definition This PR creates a new definition package. Where is GH Actions? GH Actions didn't give a response to this PR labels Jul 22, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 22, 2020

@ExE-Boss Thank you for submitting this PR!

This is a live comment which I will keep updated.

Code Reviews

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

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.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 46276,
  "author": "ExE-Boss",
  "owners": [],
  "dangerLevel": "ScopedAndConfiguration",
  "headCommitAbbrOid": "9cf0b9c",
  "headCommitOid": "9cf0b9cb231e368c4f657698454cf1d9811d4a97",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-28T16:09:50.000Z",
  "lastCommentDate": "2020-07-29T16:06:28.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46276/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "anyPackageIsNew": true,
  "packages": [
    "regenerator-runtime"
  ],
  "files": [
    {
      "path": "types/regenerator-runtime/index.d.ts",
      "kind": "definition",
      "package": "regenerator-runtime"
    },
    {
      "path": "types/regenerator-runtime/regenerator-runtime-tests.ts",
      "kind": "test",
      "package": "regenerator-runtime"
    },
    {
      "path": "types/regenerator-runtime/tsconfig.json",
      "kind": "package-meta",
      "package": "regenerator-runtime",
      "suspect": "created"
    },
    {
      "path": "types/regenerator-runtime/tslint.json",
      "kind": "package-meta-ok",
      "package": "regenerator-runtime"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-07-29T15:34:21.000Z",
  "reviewersWithStaleReviews": [
    {
      "reviewedAbbrOid": "0743779",
      "reviewer": "sheetalkamat",
      "date": "2020-07-23T18:27:21Z"
    }
  ],
  "approvalFlags": 4,
  "isChangesRequested": false
}

@typescript-bot
Copy link
Contributor

🔔 @ExE-Boss — 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...)

@typescript-bot typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jul 22, 2020
@typescript-bot typescript-bot moved this from Other to Needs Maintainer Review in New Pull Request Status Board Jul 22, 2020
@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

These typings are for a package that doesn’t yet exist on master, so I don’t have anything to compare against yet! In the future, I’ll be able to compare PRs to regenerator-runtime with its source on master.

Comparison details 📊
Batch compilation
Type count 3748
Assignability cache size 374
Language service measurements
Samples taken 229
Identifiers in tests 229
getCompletionsAtPosition
    Mean duration (ms) 100.0
    Mean CV 15.9%
    Worst duration (ms) 174.9
    Worst identifier mappedIterator
getQuickInfoAtPosition
    Mean duration (ms) 101.4
    Mean CV 16.2%
    Worst duration (ms) 163.5
    Worst identifier tryEntries
System information
Node version v12.18.2
CPU count 2
CPU speed 2.095 GHz
CPU model Intel(R) Xeon(R) Platinum 8171M CPU @ 2.60GHz
CPU Architecture x64
Memory 6.8 GiB
Platform linux
Release 4.15.0-1091-azure

export function async<T = undefined, TReturn = unknown>(
innerFn: InnerFunction<T, unknown, TReturn>,
// tslint:disable-next-line: ban-types
outerFn?: Function | null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not disable this error:: Fix this instead by changing to () => void

Copy link
Contributor Author

@ExE-Boss ExE-Boss Jul 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That signature is incorrect, as it won’t accept newable‑only functions and anything that takes at least one required parameter.

The Function type most closely describes what the wrap and async functions need: https://github.com/facebook/regenerator/blob/0566ce53c67b5fe1de13b47ed4db00570de7d9f6/packages/regenerator-runtime/runtime.js#L39.

Technically it’s { prototype?: unknown }, but that causes:

TS2559: Type '<T, U, TNext = unknown>(this: Iterator<T, unknown, TNext>, mapper: (value: T) => U) => Generator<U, void, TNext>' has no properties in common with type '{ prototype?: unknown; }'

types/regenerator-runtime/index.d.ts Show resolved Hide resolved
@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 23, 2020
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board Jul 23, 2020
@typescript-bot
Copy link
Contributor

@ExE-Boss One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

@typescript-bot typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jul 26, 2020
@typescript-bot typescript-bot moved this from Needs Author Action to Needs Maintainer Review in New Pull Request Status Board Jul 26, 2020
@typescript-bot
Copy link
Contributor

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

@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner labels Jul 29, 2020
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Author to Merge in New Pull Request Status Board Jul 29, 2020
@typescript-bot
Copy link
Contributor

@ExE-Boss Everything looks good here. Great job! I am ready to merge this PR on your behalf.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

@ExE-Boss
Copy link
Contributor Author

Ready to merge

@typescript-bot typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board Jul 29, 2020
@typescript-bot typescript-bot merged commit 2bcf3bd into DefinitelyTyped:master Jul 29, 2020
@ExE-Boss ExE-Boss deleted the types/regenerator-runtime/v0.13 branch July 29, 2020 16:07
@typescript-bot typescript-bot removed this from Recently Merged in New Pull Request Status Board Jul 30, 2020
astorije pushed a commit to astorije/DefinitelyTyped that referenced this pull request Jul 30, 2020
… definition by @ExE-Boss

* feat: Add `regenerator‑runtime` type definition

* fix(regenerator‑runtime/mark): Preserve overloads

* fix(regenerator‑runtime): Improve order of `context.abrupt(…)` inference

* fix(regenerator‑runtime): Fix `AsyncIterator.next` signature
danielrearden pushed a commit to danielrearden/DefinitelyTyped that referenced this pull request Sep 22, 2020
… definition by @ExE-Boss

* feat: Add `regenerator‑runtime` type definition

* fix(regenerator‑runtime/mark): Preserve overloads

* fix(regenerator‑runtime): Improve order of `context.abrupt(…)` inference

* fix(regenerator‑runtime): Fix `AsyncIterator.next` signature
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintainer Approved New Definition This PR creates a new definition package. Self Merge This PR can now be self-merged by the PR author or an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants