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

Open question: Promise[Symbol.matcher] #264

Closed
Tracked by #175
ljharb opened this issue Jul 8, 2022 · 7 comments · Fixed by #293
Closed
Tracked by #175

Open question: Promise[Symbol.matcher] #264

ljharb opened this issue Jul 8, 2022 · 7 comments · Fixed by #293

Comments

@ljharb
Copy link
Member

ljharb commented Jul 8, 2022

(from #263)

Currently, it uses the IsPromise AO, just like Promise.resolve does.

Should we loosen this to also accept thenables?

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

My personal opinion is no. With the current semantics:

match (value) {
  when (Promise) { /* real Promise */ }
  when (Promise | { then: Function }) {/* thenable */ }
}

With loosened semantics:

function isPromise(value) {
  const p = Promise.resolve(p);
  p.then(() => {}, () => {}); // to avoid unhandled rejections
  return value === p;
}
match (value) {
  when (Promise) if (isPromise(value)) { /* real Promise */ }
  when (Promise) {/* thenable */ }
}

The latter seems much worse to me than the former.

@ljharb ljharb mentioned this issue Jul 8, 2022
28 tasks
@theScottyJam
Copy link
Contributor

theScottyJam commented Jul 8, 2022

I would agree here - I would expect to be able to write code like this, and for it to just work:

async function doThing(value) {
  return match (value) {
    when (${Promise}): value.catch(...)
    default: value
  }
}

It would be odd if someone could pass in a then-able and have this function crash because the thenable doesn't implement a .catch() method.

Plus, it's trivial to check if something is a thenable if that's really what's wanted, as you just showed.

@Jack-Works
Copy link
Member

Maybe we can try to convert a then-able to a Promise.

Promise[@@matcher] = function (value) {
    const f = value.then
    if (typeof f !== 'function') return { matched: false }
    return { matched: true, value: Promise.resolve(value) }
}

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

What would be the benefit? If they want a real promise, they'll Promise.resolve the thenable, or just await it, no?

@tabatkins
Copy link
Collaborator

Also agree on no - it would be surprising if Promise, alone among the built-ins, accepted random objects that weren't Promise objects.

If authors want to handle thenables they can do so themselves. The thenable-handling should be considered a legacy-compatibility thing (we did it to help early adoption, as it gave us compatibility with userland Promise libraries), and not something we care about going forward except for consistency in some cases.

@ljharb
Copy link
Member Author

ljharb commented Jul 11, 2022

cc @mpcsh @DanielRosenwasser @rkirsling @codehag; any thoughts? If we have consensus on "no" then this can be closed by removing the TODO in the spec.

@rkirsling
Copy link
Member

I also agree with "no". 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants