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(react-dom): Add async act #37554

Merged
merged 3 commits into from Aug 19, 2019

Conversation

@eps1lon
Copy link
Contributor

commented Aug 12, 2019

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).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: Ports #37426 to react-dom/test-utils
  • 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 you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

@eps1lon eps1lon requested a review from Jessidhia as a code owner Aug 12, 2019

@eps1lon eps1lon referenced this pull request Aug 12, 2019
3 of 3 tasks complete
@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #37554 diff
Batch compilation
Type count 52913 53044 +0.2%
Assignability cache size 52867 52903 +0.1%
Subtype cache size 236 250 +5.9%
Identity cache size 92 89 -3.3%
Language service
Samples taken 1835 367 -80.0%
Identifiers in tests 367 367 0.0%
getCompletionsAtPosition
    Mean duration (ms) 629.7 635.9 +1.0%
    Median duration (ms) 624.5 626.3 +0.3%
    Mean CV 7.6%
    Worst duration (ms) 807.1 803.9 -0.4%
    Worst identifier React React
getQuickInfoAtPosition
    Mean duration (ms) 568.2 563.5 -0.8%
    Median duration (ms) 549.3 555.8 +1.2%
    Mean CV 8.7%
    Worst duration (ms) 787.0 728.9 -7.4%
    Worst identifier createElement createPortal
System information
Node version v10.15.3 v10.16.1
CPU count 2 2
CPU speed 2.294 GHz 2.394 GHz
CPU model Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz
CPU Architecture x64 x64
Memory 6.8 GiB 6.8 GiB
Platform linux linux
Release 4.15.0-1041-azure 4.15.0-1052-azure

First off, note that the system varied slightly between these two runs, so you’ll have to take these measurements with a grain of salt.

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@eps1lon Thank you for submitting this PR!

🔔 @MartynasZilinskas @theruther4d @Jessidhia - 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.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

export function act(callback: () => void | undefined): DebugPromiseLike;
// the "void | undefined" is here to forbid any sneaky return values
// tslint:disable-next-line: void-return
export function act(callback: () => Promise<void | undefined>): Promise<undefined>;

This comment has been minimized.

Copy link
@davidje13

davidje13 Aug 12, 2019

Contributor

I don't think this needs multiple signatures; DebugPromiseLike is promise-like, after all:

export function act(callback: () => Promise<void | undefined> | void | undefined): DebugPromiseLike;

should be fine. And I'm not sure if it even needs the whole void | undefined thing now that it doesn't need to catch promises in a special way. Maybe getting rid of the undefineds and just using voids is fine now? I don't really understand how that bit was working before. It could also be changed to simply () => any, but that doesn't get the intended semantics across so well.

This comment has been minimized.

Copy link
@eps1lon

eps1lon Aug 12, 2019

Author Contributor

DebugPromiseLike is here to throw type errors because it will trigger react warnings if you await the sync act and more importantly can cause subtle scheduling bugs.

() => any is not valid either because react will warn if you return anything but a thenable from it.

This comment has been minimized.

Copy link
@davidje13

davidje13 Aug 12, 2019

Contributor

Ah, in that case I think the first signature should become a regular promise, because the recommendation now is to await even synchronous act (if you give it a synchronous function, it may still wait for asynchronous tasks). So maybe combine them with a Promise<void> return type?

This comment has been minimized.

Copy link
@eps1lon

eps1lon Aug 12, 2019

Author Contributor

because the recommendation now is to await even synchronous act

Do you have a link for that? I remember a PR where the author for async act explicitly told me this can cause bugs and I'm fairly certain this still issues warnings from react.

This comment has been minimized.

Copy link
@davidje13

davidje13 Aug 13, 2019

Contributor

Hmm, looks like I made it up, which is weird because I distinctly remember seeing that from a maintainer in the original tracker. Maybe it was an early thought which got edited out. Anyway doesn't seem to be the case, so I guess it does have to be 2 separate signatures.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Review in Pull Request Status Board Aug 17, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@sheetalkamat sheetalkamat merged commit 4b81edc into DefinitelyTyped:master Aug 19, 2019

3 checks passed

DefinitelyTyped.BenchmarkPR Build #11655 succeeded
Details
DefinitelyTyped.DefinitelyTyped Build #20190812.10 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pull Request Status Board automation moved this from Review to Done Aug 19, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

I just published @types/react-dom@16.9.0 to npm.

@eps1lon eps1lon deleted the eps1lon:feat/react-dom-act-async branch Aug 20, 2019

life777 added a commit to life777/DefinitelyTyped that referenced this pull request Sep 11, 2019
feat(react-dom): Add async act (DefinitelyTyped#37554)
* feat(react-dom): Add async act typings

* Update version

* Normalize typing with react-test-renderer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.