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

Better typings for Promise.resolve(), like #31117 #33074

Merged
merged 4 commits into from
Aug 2, 2022

Conversation

jablko
Copy link
Contributor

@jablko jablko commented Aug 25, 2019

No description provided.

@sandersn sandersn moved this from Waiting on reviewers to Waiting on author in PR Backlog May 24, 2022
@sandersn
Copy link
Member

sandersn commented Jun 1, 2022

@rbuckton Is this ready to merge now?

@rbuckton
Copy link
Member

rbuckton commented Aug 1, 2022

I'd like to run the RWC and user tests once more to be sure.

@rbuckton
Copy link
Member

rbuckton commented Aug 1, 2022

@typescript-bot run dt
@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 1, 2022

Heya @rbuckton, I've started to run the diff-based user code test suite on this PR at 915863f. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 1, 2022

Heya @rbuckton, I've started to run the parallelized Definitely Typed test suite on this PR at 915863f. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 1, 2022

Heya @rbuckton, I've started to run the extended test suite on this PR at 915863f. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Heya @rbuckton, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

@typescript-bot
Copy link
Collaborator

@rbuckton
Great news! no new errors were found between main..refs/pull/33074/merge

@rbuckton
Copy link
Member

rbuckton commented Aug 2, 2022

This still causes a build break in older versions of firebase-sdk, but since this only occurs in an outdated snapshot of firebase in our RWC tests and not in the most recent version of firebase, this change seems acceptable.

    /**
     * Takes an array of values and sequences them using the promise (or value)
     * returned by the supplied callback. The callback for each item is called
     * after the promise is resolved for the previous item.
     * The function returns a promise which is resolved after the promise for
     * the last item is resolved.
     */
    export function sequence<T, R>(
      values: T[],
      fn: (value: T, result?: R) => R | Promise<R>,
      initialValue?: R
    ): Promise<R> {
      let result = Promise.resolve(initialValue);
      values.forEach(value => {
        result = result.then(lastResult => fn(value, lastResult));
        ~~~~~~
!!! error TS2322: Type 'Promise<R | Awaited<R>>' is not assignable to type 'Promise<Awaited<R>>'.
!!! error TS2322:   Type 'R | Awaited<R>' is not assignable to type 'Awaited<R>'.
!!! error TS2322:     Type 'R' is not assignable to type 'Awaited<R>'.
      });
      return result as AnyDuringMigration;
    }

@rbuckton rbuckton merged commit 040c121 into microsoft:main Aug 2, 2022
PR Backlog automation moved this from Waiting on author to Done Aug 2, 2022
@sandersn
Copy link
Member

sandersn commented Aug 3, 2022

Note that this breaks @types/react-test-renderer's attempt to create a type that can't be passed to Promise.resolve:

declare const UNDEFINED_VOID_ONLY: unique symbol;
export function act(callback: () => void | { [UNDEFINED_VOID_ONLY]: never }): DebugPromiseLike;

// Intentionally doesn't extend PromiseLike<never>.
// Ideally this should be as hard to accidentally use as possible.
export interface DebugPromiseLike {
    // the actual then() in here is 1-ary, but that doesn't count as a PromiseLike.
    then(onfulfilled: (value: never) => never, onrejected: (reason: never) => never): never;
}
// @ts-expect-error <-- this is no longer an error
Promise.resolve(act(() => {}));

I think this change is still OK, I just wanted to report on the break.

@sandersn
Copy link
Member

sandersn commented Aug 8, 2022

This breaks vscode in a few places too, so we're going to revert it for 4.8 to give us a chance to figure out what to do.

sandersn added a commit that referenced this pull request Aug 8, 2022
typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 9, 2022
Component commits:
c28ee65 Revert "Better typings for Promise.resolve(), like microsoft#31117 (microsoft#33074)"
This reverts commit 040c121.
typescript-bot added a commit to typescript-bot/TypeScript that referenced this pull request Aug 11, 2022
Component commits:
c28ee65 Revert "Better typings for Promise.resolve(), like microsoft#31117 (microsoft#33074)"
This reverts commit 040c121.

Co-authored-by: Nathan Shively-Sanders <nathansa@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants