paldepind / union-type

A small JavaScript library for defining and using union types.
MIT License
477 stars 28 forks source link

caseOn with _ has variable argument count #24

Closed kurtharriger closed 8 years ago

kurtharriger commented 8 years ago

In the caseOn example none of the values had any arguments, so player is always the first argument. However, if you had an additional type that took an argument then player may or may not be the first argument. I believe this is caused by action.concat[arg] here: https://github.com/paldepind/union-type/blob/master/union-type.js#L55

Perhaps it would make sense for to just skip the concat when matching on ? Perhaps it could even pass the raw action array as first argument or as context, but I'm not sure if it would be useful when you don't otherwise know what it is.

paldepind commented 8 years ago

However, if you had an additional type that took an argument then player may or may not be the first argument.

In which cases? The player should always be the last argument.

kurtharriger commented 8 years ago

Yes, the player will always be the last argument. This is certainly workable, but it feels slightly less consistent and less expressive. Since _ matches may match types constructors with different arity you can't simply use (player) => player as an identity.

For example, if we add Jump: [] and want this and all future actions to be ignored as an identity function on the player we might expect to do this:

var Move = Type({Up: [], Right: [], Down: [], Left: [], Jump: [], Fire: [Number]});

var player = {x: 0, y: 0};
const advancePlayer = Move.caseOn({
  Up: (player) => ({x: player.x, y: player.y - 1}),
  Right: (player) => ({x: player.x + 1, y: player.y}),
  Down: (player) => ({x: player.x, y: player.y + 1}),
  Left: (player) => ({x: player.x - 1, y: player.y}),
  _: (player) => player
});

However, when we later add Fire: [Number] we will discover the above no longer works as expected since player is the second rather than the first argument for Fire. You also can't use (_, player) => player as that would only work for Fire but not for Jump.

The best current solution is to use R.last as the identity function instead and the following does work:

var Move = Type({Up: [], Right: [], Down: [], Left: [], Jump: [], Fire: [Number]});

const advancePlayer = Move.caseOn({
  Up: (player) => ({x: player.x, y: player.y - 1}),
  Right: (player) => ({x: player.x + 1, y: player.y}),
  Down: (player) => ({x: player.x, y: player.y + 1}),
  Left: (player) => ({x: player.x - 1, y: player.y}),
  _: R.last
});

But my thinking is that if you are matching on _ than the type is essentially opaque and destructuring only serves to mess up argument order potentially breaking existing code like in the first example.

So I propose that when matching _ the value is not destructured at all since if you don't know the type then you don't know what the arguments are either.

Perhaps there might be cases where you still want to do something with the raw value in an opaque way, such as call some other function that accepts a move parameter. Case statements that know the type could reconstruct it, but _ can not. If move was provided as a single argument then It could perhaps call another function that takes a move, but if it doesn't know the type it can't reconstruct it. For this use case I would propose that all case statements are provided the raw value as well. It might make sense to provide the raw value as the last argument to all functions, but that would probably code like mine using R.last. So I think the best way to provide the raw value is via this.

paldepind commented 8 years ago

Ok. I understand much better now. I get what you are saying and can certainly see the issue.

What about doing something like this when you need the raw value:

const ex = (value) => {
  return SomeType.case({
    First: (x) => someFn(value) + x,
    Second: () => anotherFn(value),
  });
}

It's not super pretty. But I don't think using this is very nice either. I'm not sure though.

Then for _ the matched value would not be passed at all to the handling function.

kurtharriger commented 8 years ago

Sounds good.

I currently don't need the raw value in caseOn and if I ever did I would be fine switching case. this is often poorly understood and binding issues cause unnecesary problems. Using a closure is more explicit and doesn't have any of these issues.