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

Confirm missing constructor matchers #266

Open
5 tasks
Tracked by #175
ljharb opened this issue Jul 8, 2022 · 4 comments
Open
5 tasks
Tracked by #175

Confirm missing constructor matchers #266

ljharb opened this issue Jul 8, 2022 · 4 comments

Comments

@ljharb
Copy link
Member

ljharb commented Jul 8, 2022

(from #263)

  • Iterator/AsyncIterator (probably when Iterator Helpers advances)
  • Record/Tuple (definitely when R&T advances)
  • GeneratorFunction/AsyncFunction/AsyncGeneratorFunction (maybe)
  • Generator/AsyncGenerator (probably not)
  • Proxy (definitely not)
@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

I doubt the last two are controversial as a "no", and I doubt the first two are controversial for a "yes".

I'd lightly prefer to add the third one, but I don't feel strongly about it if anyone is strongly opposed.

@ljharb ljharb mentioned this issue Jul 8, 2022
28 tasks
@Jack-Works
Copy link
Member

GeneratorFunction/AsyncFunction/AsyncGeneratorFunction

I'm skeptical about this. This allows devs to "match" an async function from a regular function, which is an implementation detail. It will cause a refactoring hazard (you should be able to switch between return new Promise and async function meanwhile not breaking anything).

Proxy

I suggest adding a @@matcher that always throws to prevent the use of it. It also applies to all other NO-matchers. TypeScript can mark them as [Symbol.matcher]: never to avoid its use of it.

Record/Tuple (definitely when R&T advances)

Should we add a record&tuple matcher that only matches R&T? Or devs will do this?

when (${Record} & { a, b }): console.log('record', a, b)
when (${Object} & { a, b }): console.log('record', a, b)

Iterator/AsyncIterator

Should it do an internal slot check, a shape check, or a type coherence (Iterator.from(value))?

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

You can already check that - https://npmjs.com/is-async-function, https://npmjs.com/is-generator-function, etc - it's just not ergonomic. It's fair tho that perhaps we shouldn't encourage such checking.

For Proxy, I suppose that makes sense, altho without a custom matcher it'll just check for ===, so it'd be pretty useless.

I think devs will definitely do that. We'll also want prototype matchers for boxed records and tuples, just like I added for other boxed primitives.

For Iterator, I would expect it to do an internal slot check - to only pass on things that Iterator.from returns, iow.

@theScottyJam
Copy link
Contributor

I'm skeptical about this. This allows devs to "match" an async function from a regular function, which is an implementation detail. It will cause a refactoring hazard (you should be able to switch between return new Promise and async function meanwhile not breaking anything).

While generally not recommended, there are scenarios where it can be nice to detect the difference between the two.

For example, I once wrote a library that provided doThing() and doThingAsync() functions. The doThing() function accepted a syncrounous function, and would return a result syncrounously. The doThingAsync() function accepted an asyncounous function and would return a promise. If you tried passing an asyncrounous function into doThing(), you'll end up with silent bugs in your code. To help prevent this, I added a check into doThing() to try and detect if the passed-in function was an async function or not, and if was async I would throw an error, letting you know you were calling the function incorrectly. I did this check by checking if the provided function was an instance of AsyncFunction. I knew that this check wasn't bullet-proof, and that it would fail to detect synchronous functions that returned promises, but I figured it's better to sometimes inform people about their bugs, even if I can't reliably detect them, then to never do it.

Considering that GeneratorFunction, AsyncFunction, and AsyncGeneratorFunction are all already not available on the global scope, and you have to jump through hoops to get to it, I would expect that a developer who's trying to do the sort of detection I did would run into warnings on the internet about the dangers of these detections, letting them know to be careful. Because these extra hoops are already in place, and because I believe there is the occasional valid reasons to do such a check, I think it's best to let these functions implement the matcher symbol, to let you do this sort of detection with pattern matching if you choose to do so. It would keep their behaviors consistent with what would be expected as well.

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

No branches or pull requests

3 participants