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

R.merge and type intersection #57

Closed
pzavolinsky opened this issue May 12, 2016 · 5 comments
Closed

R.merge and type intersection #57

pzavolinsky opened this issue May 12, 2016 · 5 comments

Comments

@pzavolinsky
Copy link

Hi, I'm having problems with R.merge(a,b) where b is a subset of a:

interface Person {
  first: string,
  last: string
};
function setLast(last: string, person: Person) : Person {
  return R.merge(person, { last });
}

The code above complains with:

error TS2322: Type '<T1>(a: T1) => T1 & { last: string; }' is not assignable to type 'Person'.
  Property 'first' is missing in type '<T1>(a: T1) => T1 & { last: string; }'.

If I explicitly state T1 and T2, the code works but it gets very ugly:

function setLast(last: string, person: Person) : Person {
  return R.merge<Person, { last:string }>(person, { last });
}

Most of the time I use R.merge to update existing properties of immutable objects (like last in the example above). I think support for this use-case should be straight-forward. Worst case, this might be OKish:

R.merge<Person>(person, { last }) : Person & { last:string } // == Person

Having to specify the type of { last } feels like an unnecessary pain.

On a side note, for the use-case when I'm augmenting a type (i.e. A & B != A) I have no problem defining the new type.

@pzavolinsky
Copy link
Author

More info on this.

It looks like the problem is a conflict in these two definitions:

merge<T2>(a: R.placeholder, b: T2): <T1>(a: T1) => T1&T2;
merge<T1, T2>(a: T1, b: T2): T1 & T2;

Removing the first one fixes the issue.

I recommend reviewing (and possibly removing) the R.placeholder variation in favor simplifying the most common use-case (at least in my experience) of using R.merge with one or two objects.

Incidentally, any R.placeholder scenario can be replaced with a R.flip(R.merge).

@donnut
Copy link
Collaborator

donnut commented May 20, 2016

I just came up with the same finding after 8 days ;)
This works indeed.

The placeholder thing is something I'm not happy with at all. It doesn't fit typescript well, although I do see its use-case. It is probably better to make a clear choice and just not support them at all.

@pzavolinsky
Copy link
Author

The placeholder thing is something I'm not happy with at all. (...) It is probably better to make a clear choice and just not support them at all.

Agreed, we decided not to use R.__ at all in our codebase (since it felt more obscure than the R.flip alternative).

@donnut
Copy link
Collaborator

donnut commented May 20, 2016

I'll shoot a PR with the placeholders removed. If there are no serious objections, we remove them definitely.

@pzavolinsky
Copy link
Author

Cool @donnut , thanks!

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

No branches or pull requests

2 participants