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

fix(ext/webidl): change createPromiseConverter #16367

Merged
merged 13 commits into from May 18, 2023

Conversation

marcosc90
Copy link
Contributor

This commit fixes a few of the missing WPTs for Streams.

It also fixes the remaining two tests in #16276 to make it 39/39

@marcosc90 marcosc90 changed the title fix(ext/streams): change createPromiseConverter fix(ext/webidl): change createPromiseConverter Oct 20, 2022
@bartlomieju bartlomieju added this to the 1.27 milestone Oct 22, 2022
Comment on lines 838 to 842
// should be able to handle thenables
// see: https://github.com/web-platform-tests/wpt/blob/a31d3ba53a79412793642366f3816c9a63f0cf57/streams/writable-streams/close.any.js#L207
typeof V?.then === "function"
? PromisePrototypeThen(PromiseResolve(V), (V) => converter(V, opts))
: PromiseResolve(converter(V, opts));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this differs from the existing code. PromiseResolve(V) already turns a thenable into a native Promise. Can you elaborate on what this is meant to do?

Copy link
Contributor Author

@marcosc90 marcosc90 Oct 23, 2022

Choose a reason for hiding this comment

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

This change fixes the tests because the current logic (main) always adds an extra Promise by calling .then, see the following snippet


This patch:

function promiseConverter(p) {
  return Promise.resolve(p);
}

const p1 = new Promise(resolve => resolve()).then(() => console.log('1'));
const p2 = promiseConverter(undefined).then(() => console.log('2'));
const p3 = new Promise(resolve => resolve()).then(() => console.log('3'));
// 1, 2, 3

main

function promiseConverter(p) {
  return Promise.resolve(p).then(() => undefined);
}

const p1 = new Promise(resolve => resolve()).then(() => console.log('1'));
const p2 = promiseConverter(undefined).then(() => console.log('2'));
const p3 = new Promise(resolve => resolve()).then(() => console.log('3'));

// 1, 3, 2

So the code checks whether it's a thenable or not to avoid calling .then if it's not, and just call Promise.resolve

@bartlomieju bartlomieju removed this from the 1.27 milestone Oct 24, 2022
@aapoalas aapoalas enabled auto-merge (squash) May 18, 2023 10:07
@aapoalas aapoalas merged commit b0f1356 into denoland:main May 18, 2023
11 checks passed
@marcosc90 marcosc90 deleted the fix-webidl-promise-converter branch August 12, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants