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.

Sign up for GitHub

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

Jump to bottom

fix(ext/webidl): change createPromiseConverter #16367

Merged
merged 13 commits into from May 18, 2023
Merged

fix(ext/webidl): change createPromiseConverter #16367

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
133dcfe
fix(ext/streams): change createPromiseConverter to be spec compliant
marcosc90 Oct 20, 2022
3c5655e
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Oct 28, 2022
51c7972
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Oct 28, 2022
a71c765
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Oct 29, 2022
acd3cda
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Oct 31, 2022
08e978a
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Nov 11, 2022
1309963
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Nov 12, 2022
75cd2f2
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Nov 15, 2022
5eb28c7
Merge branch 'main' into fix-webidl-promise-converter
marcosc90 Dec 11, 2022
d0d2e48
Merge branch 'main' into fix-webidl-promise-converter
aapoalas Mar 25, 2023
2883c13
Merge branch 'main' into fix-webidl-promise-converter
aapoalas May 18, 2023
424aa0a
Fix lint
aapoalas May 18, 2023
65abd77
Fix expectations
aapoalas May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion ext/webidl/00_webidl.js
View file
Open in desktop
Expand Up @@ -835,7 +835,11 @@

function createPromiseConverter(converter) {
return (V, opts) =>
PromisePrototypeThen(PromiseResolve(V), (V) => converter(V, opts));
// 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 •
edited

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

}

function invokeCallbackFunction(
Expand Down
18 changes: 4 additions & 14 deletions tools/wpt/expectation.json
View file
Open in desktop
Expand Up @@ -1296,14 +1296,8 @@
"bad-buffers-and-views.any.worker.html": true,
"construct-byob-request.any.html": true,
"construct-byob-request.any.worker.html": true,
"general.any.html": [
"ReadableStream with byte source: Respond to multiple pull() by separate enqueue()",
"ReadableStream with byte source: enqueue() discards auto-allocated BYOB request"
],
"general.any.worker.html": [
"ReadableStream with byte source: Respond to multiple pull() by separate enqueue()",
"ReadableStream with byte source: enqueue() discards auto-allocated BYOB request"
],
"general.any.html": true,
"general.any.worker.html": true,
"non-transferable-buffers.any.html": false,
"non-transferable-buffers.any.worker.html": false,
"enqueue-with-detached-buffer.window.html": false,
Expand Down Expand Up @@ -1372,12 +1366,8 @@
"bad-underlying-sinks.any.worker.html": true,
"byte-length-queuing-strategy.any.html": true,
"byte-length-queuing-strategy.any.worker.html": true,
"close.any.html": [
"when close is called on a WritableStream in waiting state, ready should be fulfilled immediately even if close takes a long time"
],
"close.any.worker.html": [
"when close is called on a WritableStream in waiting state, ready should be fulfilled immediately even if close takes a long time"
],
"close.any.html": true,
"close.any.worker.html": true,
"constructor.any.html": true,
"constructor.any.worker.html": true,
"count-queuing-strategy.any.html": true,
Expand Down