-
Notifications
You must be signed in to change notification settings - Fork 89
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
overhauled proposal #65
Conversation
58ec0e0
to
961c234
Compare
I also prefer the In general, as a middle-talented JavaScript dev, I find the new syntax & examples easier to pick up than the previous syntax. cheers |
Re. |
bc0cab0
to
c5c370e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just my two cents :)
README.md
Outdated
} | ||
|
||
match (2) { | ||
x if (x === y) => x === y === 2 // guard comparison with variable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will evaluate to false
, because x === y
is true, but true === 2
is not. Not sure if intended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was originally unintentional but then I just left it because I figured it would get the point across. I may as well make it be actually valid, though.
README.md
Outdated
```js | ||
match (x) { | ||
{x: 1, y} => ..., // the y property is required, and is locally bound to y | ||
{} => ..., // matches any object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you match an empty object then? Shouldnt { ..._}
(where _
is a normal variable name) match any object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the common case, by far, is for keys on objects to be optional, and forcing matches to be exhaustive would defeat the purpose of this kind of destructuring. I feel like special-casing only {}
might be surprising. If you really wanna make sure it's an empty object, x if (!Object.keys(x).length) => ...
would do that for you.
ec8466c
to
7c25619
Compare
README.md
Outdated
|
||
This document includes a [detailed description of the `match` API](#api), as | ||
well as conscious [design decisions](#design-decisions) that were made involving | ||
various details. Further down, you can also fine a section on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine → find
README.md
Outdated
|
||
{type: 'toggle-todo', index} => ({ | ||
todos: state.todos.map((todo, idx) => idx === index | ||
? Object.assign({}, todo, {done: !todo.done}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These examples with Object.assign would probably be more representative if they used object spread
README.md
Outdated
|
||
#### <a name="primitive-matcher"></a> > Primitives | ||
|
||
Primitive types will be matched with `===`. The following literals can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use Object.is
instead (SameValue), to properly handle NaN
and -0
README.md
Outdated
|
||
#### <a name="object-matcher"></a> > Objects | ||
|
||
Objects are destructured. Any variables mentioned in the match side MUST exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables don’t exist in objects, properties do - either as own or inherited. Can this be reworded a bit?
README.md
Outdated
{x: {y: 1}} => ..., | ||
{x, ...y} => ..., // binds all-other-properties to `y`. | ||
{x, ...{y}} => ..., // SyntaxError | ||
Foo {y} => ...,// matches an instance of `Foo` or, if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default Symbol.patternMatch on Object.prototype that’s instanceof
, or is this the default behavior?
Separately, instanceof
doesn’t work cross-realm, so I’m uncomfortable with relying on it as default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is worth talking about -- right now, the check is simply whether or not a Symbol.patternMatch
method is available to the object at all -- and the instanceof
fallback only happens if that is true and Foo
is typeof 'function'
.
I agree that instanceof
is a massive pain, specially around realms, but there's no generic "this type of thing" check that would facilitate what I consider a pretty important/common use case for extractors (basic class checks). So I kept it in there because the alternative is worse, imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that when such a mechanism exists - like @jasnell’s “builtins.is” proposal - that would be what we’d want as the default matching mechanism, not instanceof, and it might not be a change that’s possible to make later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any ideas on how to have our cake and eat it too, with this? I think it'd be a really big loss for something like this if everyone is required to add their own method to every single class they make, just to do a type match, and I'm not sure what the status of builtins.is
is -- if that ships first, that would solve this problem, though, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved this into a bikeshed section so it can be addressed later.
README.md
Outdated
the very very different `switch` semantics, and also has a very clear symmetry | ||
that allows `&&` to work just fine. It also fits with other pattern matching | ||
engines that allow alternatives like this actually do. I don't believe this is | ||
worth further bikeshedding. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the dissonance with the existing value selection operators, and the nullish coalescing operator, will necessitate further discussion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what dissonance you're referring to here (see above comment about them working off match-success, not actual values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a dev will expect && anywhere to work like && everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure, since ||
doesn't make sense on the LHS of assignments already, but I'm willing to hear more options.
We could do what Erlang does and make it depend on ;
or ,
or something ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I would actually be very interested in finding ways to get more feedback from the community about this and other similar syntax bikesheds. I don't actually think there's an objective answer to this besides actual research -- and I've gotten a sense of positive responses about this construct so far from anyone who's noted it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but for the sake of tossing stuff in the mix:
1 | 2 | 3 => ...
and {x: 1}, {y: 2} => ...
(for or
and and
, respectively) seem like they could be reasonable alternatives maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former collides with (the admittedly way less used) bitwise OR, but a comma separated list, or keywords like or
and and
seem like they’d work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust uses |
for pattern-match OR, and also has |
as the bitwise operator (and ||
is a boolean OR). I'm even more comfortable with overloading bitwise here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've added a bikeshed section about this to the document so we can take this discussion out of a github issue thread: https://github.com/zkat/proposal-pattern-matching/blob/master/README.md#compound-matcher-syntax
README.md
Outdated
} | ||
``` | ||
|
||
It is possible that array patterns could also match iterables, however it's not clear this design is desirable. First, it's not clear that users of pattern matching would expect array patterns to match any object with Symbol.iterable. Second, since pulling values out of an iterable is side-effecty, it introduces a lot of confusion around what the state of the iterable is in each match leg and it's not clear that side-effecty-by-default pattern matching is a good idea. | ||
Another argument in favor of the special case is so, instead of [making `match` | ||
automatically pun on `null` equality](null-punning), we can use `||` to do the equivalent of a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the definition of the term “pun” here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like "use a separate, non-obvious construct for the sake of a convenient syntax".
For example: foo = foo || 1
for defaults, or x == null
to just generally check for "things that are not there". They're both perfectly valid examples, but they rely on some cleverness for the sake of terseness. It's a term I've seen used elsewhere in language design discussions when it comes to such features. (see also, nil
-punning in lisps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, TIL
README.md
Outdated
} | ||
``` | ||
|
||
## Identifier Patterns & Symbol.matches | ||
Or, alternatively, using `|` if [`undefined` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
||
?
README.md
Outdated
|
||
### Nested Matching | ||
Because match is an expression, you can match further in the consequent of a match leg. Consider: | ||
Another benefit of using `@` is that it could allow folks to use an extractor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s worth mentioning the conflict with decorators here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
README.md
Outdated
##### Option D | ||
|
||
Similarly to Option C, the "magic constructor" could be stored in | ||
`Symbol.patternMatch` itself, and defining successful matches as anything that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an option e, where we add an actual Maybe-like primitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add that, but it feels like a bit of a chicken-or-egg problem where a Maybe
-like primitive is much less useful without a match
-like construct. I'll definitely add this, because if that's on the table, I am ALL OVER THAT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure if it is or not, but I’ve been casually asking delegates about it for a few years on and off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth pushing harder on it, though, since this would really benefit from that.
README.md
Outdated
throw new Error("Unknown vector type"); | ||
function todoApp (state = initialState, action) { | ||
const newState = match (action) { | ||
{type: 'set-visibility-filter', filter: visFilter} => ({visFilter}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this syntax work with matching Symbols and computed properties? Is it just a normal destructuring pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working off the assumption that {[Symbol.whatnot]: 'value'}
would work just as well here, yes, since it works in regular destructuring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a note to the object-matcher
section to make this explicit.
Champions: Brian Terlson (Microsoft, [@bterlson](https://twitter.com/bterlson)), Sebastian Markbåge (Facebook, [@sebmarkbage](https://twitter.com/sebmarkbage)), Kat Marchán (npm, [@maybekatz](https://twitter.com/maybekatz)) | ||
|
||
This proposal adds a new feature to ECMAScript, called "Pattern Matching", | ||
through a new expression statement named `match`. Pattern matching is a feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I've missed this, but what about using match
as a variable name in a block? It's common to assign the result array returned by the match()
method to a local variable named match
.
const re = /([\w ]+)::(\d{1,2} \w+ \d{4}).*/i;
const match = 'USA::14 February 2018 (internet)'.match(re);
Will it work or throw the SyntaxError: Cannot use the keyword 'match' as a lexical variable name.
exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding, specially considering the cover grammar comment in the syntax outline, is that match
is not a reserved word but will actually still work when a lexical binding exists? Cc others who know this part better than me
I had some discussions about the initial design of this proposal, but I decreased my participation because I didn't feel it was being listened, even having many "likes" at some opinions (of course popularity doesn't mean being right). Now, almost a year later, I'm glad seeing this impressive work, congratulations @zkat 👏 👏 👏, amazing job, and I feel good that somehow the discussions were productive (and finally the damn "else leg" was removed! haha 😂). |
Rendered text
After having some conversations with @tabatkins, @bterlson, @wycats, and others, I've put together a new proposal that I believe addresses a large swath of issues that were being discussed with the previous one.
It is largely the same proposal, with a few new features (such as Extractors), new semantics for matching, and slightly different syntax cause I like it better.
Everything is pretty thoroughly documented (considering this is a first draft), and I've made notes both about executive design decisions I made in the redesign, as well as some fields ripe for the bikeshedding that I believe are worth discussing.
To go with this proposal, I've also written an npm package that works as a regular JavaScript library -- it lacks some of the syntactic allowances, but is semantically identical to this proposal and the mapping between the sugared and desugared versions is consistent and straightforward, so folks would be able to play with it.
Also this is totally my first championed proposal since I became a delegate and I'm pretty damn excited 🙈 Thanks y'all for your support! I've wanted this feature myself for a loooong time, so I care a lot about trying to see it through to stage 4 💚