Remove optionality for Promise resolve callback #39817
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot run dt |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@RyanCavanaugh, @weswigham, @DanielRosenwasser: This one is interesting because its an improvement for TypeScript code, but results in "breaks" for JavaScript code. In TS, the expectation is that Removing optionality for |
|
Hm.... I mean, |
|
@DanielRosenwasser, @RyanCavanaugh: I'm still interested in whether this is considered a "break" and if it should wait for 4.1, or if we should consider @weswigham's suggestion to extend the |
|
This late in the release, I'd really prefer waiting until 4.1 for anything that feels close to a breaking change. |
Does that mean that #29131 has been fixed? |
|
After discussing it in the design meeting, the suggestion to make trailing |
|
@typescript-bot user test this |
|
From the user tests: office-ui-fabricThis is due to the fact that the promise created here has no type argument and is inferred to xterm.jsThis is again due to an untyped adonis-frameworkThis is due to the fact this is a .js file and our user tests compile js files with chrome-devtools-frontendIn these cases, the promises need an appropriate contextual type. Some are typed as npmThis is again due to the fact we compile our JS user tests using puppeteer
@DanielRosenwasser, @RyanCavanaugh: Regarding our existing support for treating trailing I'm also considering the possibility of giving an async function foo() {
// type is `Promise<unknown>`.
const x = new Promise(() => {});
// type is `Promise<void>`
await new Promise(() => {});
}The result would be that some of the above errors would get the correct contextual type when the result is unused. I can create a separate PR for this case, which we can possibly discuss at the next design meeting. |
|
I've filed #40227 for the assignability related issue. |
|
@typescript-bot run dt |
|
trying again... |
|
One last time, now that hopefully dtslint-runner is fixed... |
|
FYI, this is a breaking change for utility functions that externalize the export function promiseResolveReject<T = void>(): [Promise<T>, (value?: T | PromiseLike<T>) => void, (reason?: any) => void] {
let resolve: (value?: T | PromiseLike<T>) => void;
let reject: (reason?: any) => void;
const promise = new Promise<T>((resolve_, reject_) => {
resolve = resolve_;
reject = reject_;
});
return [promise, resolve!, reject!];
}Error: |
|
Yes, this is a breaking change, but one that is a change in correctness that adds type-safety. The prior definition was unsafe as it allowed you to pass Above, you would need to change CC @DanielRosenwasser to document in the 4.1 release notes that this is a breaking change intended to improve type safety. |
There has been typescript changes, where the value is required in the resolve callback for a new Promise. When resolve is called without arguments, we need to specify type void on the new Promise. More information: microsoft/TypeScript#39817
There has been typescript changes, where the value is required in the resolve callback for a new Promise. When resolve is called without arguments, we need to specify type void on the new Promise. More information: microsoft/TypeScript#39817
There has been typescript changes, where the value is required in the resolve callback for a new Promise. When resolve is called without arguments, we need to specify type void on the new Promise. More information: microsoft/TypeScript#39817
Motivation: in this version of TS, the optionality of the value passed to the Promise resolve callback does not exist. For that reason, the value passed to `resolved` is required. It can still be omitted by creating a `new Promise<void>` which is what is being done here. Read more: microsoft/TypeScript#39817
|
I just upgraded to 4.1, and I'm trying to follow the conversation here and at #40231 and #41497. My main problem is that, in code like this: const p = new Promise<number | undefined>(resolve => {
if (Math.random() > 0.5) { resolve(1); }
else { resolve(); }
});the type of I think we're stuck changing our style guide to allow passing |
|
I have this code in my codebase: it doesn't actually parameterize |
This makes
valuerequired in theresolvecallback for a newPromise. This shouldn't affect the ability to callresolve()without an argument when creating anew Promise<void>, as we already have logic in place that allows us to parameters whose types arevoidas optional.Fixes #36749