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

Curry trouble (example for R.is) #77

Closed
noobiek opened this issue Jul 25, 2018 · 14 comments
Closed

Curry trouble (example for R.is) #77

noobiek opened this issue Jul 25, 2018 · 14 comments

Comments

@noobiek
Copy link

noobiek commented Jul 25, 2018

Hi,

I understand this has to do with curry, but I constantly have trouble with R.is because of this, so...
The troube comes when I do something like this
if (R.is(Array, someVar)) { ... }
Now, if someVar is undefined then R.is will return function, so the check will pass.
This may happen in a bigger scope, when my function accepts an object A, and one of its fields B should be an array:

function myFunc(A) {
...
  if (R.is(Array, A.B)) { ... }
...
}

So, if B was not defined, the IF block will proceed as if B were an array.
I can add an additional check
if (A.B && R.is(Array, A.B)) { ... }
But that just doesn't feel and look right. Right?

@selfrefactor
Copy link
Owner

Please try it now with the latest version.

@noobiek
Copy link
Author

noobiek commented Jul 31, 2018

Hi,
Thanks! Checked out the code, looks OK.
I think that will work nicely.

@noobiek
Copy link
Author

noobiek commented Aug 9, 2018

Hi,
Would be nice to have this change for the rest of curried functions.
R.map, for examples, suffers from this situation. One could argue that it is up to user to check if supplied value is defined or undefined, but at least lodash/underscore (which I used before) don't care, they return an empty array in such cases.

@selfrefactor
Copy link
Owner

Ok, so what is your suggestion. map to return [] when undefined is provided instead of array, or I am missing something?

@noobiek
Copy link
Author

noobiek commented Aug 11, 2018

Yes, that would be consistent with how underscore/lodash work (I guess a lot of people are used to how map() and filter() work there) - return []. And this proved to be convinient, as in some cases you are not sure if some object property will be present (say, Person.PhoneNumbers can be set or not), but you don't have to check, since map() and filter() will return [] and the rest of code won't break. Without this convinience you would still do something like R.map(func, Person.PhoneNumbers||[]) each time.

But I guess in the end it must be your decision as lib architect/maintainer. At least R.map(i=>i, undefined) and R.filter(i=>i, undefined) should not return curried functions. This is definitely unexpected since I provided 2 arguments not 1 in each case.

@selfrefactor
Copy link
Owner

After such thorough explanation, I have no other option but to integrate your suggestion in the next version of Rambda.

@selfrefactor selfrefactor reopened this Aug 11, 2018
@noobiek
Copy link
Author

noobiek commented Aug 11, 2018

...so that everyone can blame me if this proves to be completely wrong. 🤣
Sure, why not!

@noobiek
Copy link
Author

noobiek commented Aug 13, 2018

Same with R.merge().
If I run R.merge({...}, undefined) I get curried function returned. Meanwhile Object.assign({a:1}, undefined) works without problems. So I have to R.merge({...}, undefined||{})....

@noobiek
Copy link
Author

noobiek commented Aug 20, 2018

Hi,
Any news/plans for this?

@selfrefactor
Copy link
Owner

It should be ready by the end of the week.

@selfrefactor
Copy link
Owner

All should be fixed with the new version. Reopen, if that is not the case.

@noobiek
Copy link
Author

noobiek commented Sep 3, 2018

Hi,

I don't think that your approach covers all cases (at least, in case of "null" it will throw an exception). Please, see how it's done in Lodash. Your code looks somewhat alike, but not quite.

function arrayMap(array, iteratee) {
    var index = -1,
        length = array == null ? 0 : array.length,
        result = Array(length);

    while (++index < length) {
      result[index] = iteratee(array[index], index, array);
    }
    return result;
}

Do you think it is better to check if arr is an array by "arr.length === undefined"?
Why not use R.is(Obect, arr) and R.is(Array, arr), for example?

@selfrefactor selfrefactor reopened this Sep 4, 2018
@selfrefactor
Copy link
Owner

I will consider your latest suggestion in the next release.

@selfrefactor
Copy link
Owner

selfrefactor commented Sep 29, 2018

I decided null to keep throwing in filter/map as otherwise Rambda will differ too much from Ramda. Imagine the surprise of fellow developers when they expect filter(fn, null) to throw and it doesn't.
undefined is different case and you make a good argument to have a saveguard for it.

I replaced arr.length === undefined with !Array.isArray(arr) if just for readability sake(thanks for pointing that out).

So I will close the issue, as version 1.2.2 addresses alread the following:

expect(R.merge(null, undefined)).toEqual({})
expect(R.map(R.add(1), undefined)).toEqual([])
expect(R.filter(Boolean, undefined)).toEqual([])

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