defaultsTo shouldn't factor in type
wagerfield opened this issue · 15 comments
As far as I can tell defaultsTo(defaultArgument, inputArgument)
is the only method that provides a way for defaulting to a value (providing some 'or' logic). However, the built-in test for inputArgument
being the same type
as defaultArgument
limits the use of this function. It is my opinion that defaultArgument
should only be returned if inputArgument
is undefined
.
I would like to compose a function works the same as Lodash's get
method or Ramda's pathOr
method whereby a value is read from a path through a nested object tree and defaults to a value if this path returns undefined
.
import { get } from 'lodash'
import { pathOr } from 'ramda'
import { curry, defaultsTo, path } from 'rambda'
// Tests written with jest
test('lodash get', () => {
expect(get({ a: { b: 2 } }, 'a.b')).toBe(2)
expect(get({ c: { b: 2 } }, 'a.b')).toBeUndefined()
expect(get({ c: { b: 2 } }, 'a.b', 'N/A')).toBe('N/A')
})
test('ramda pathOr', () => {
// From ramda's docs http://ramdajs.com/docs/#pathOr
expect(pathOr('N/A', ['a', 'b'], { a: { b: 2 } })).toBe(2)
expect(pathOr('N/A', ['a', 'b'], { c: { b: 2 } })).toBe('N/A')
})
// There's probably a more elegant way of doing this?
const rambdaPathOr = curry((defaultValue, inputPath, inputValue) =>
defaultTo(defaultValue, path(inputPath, inputValue)))
test('rambda pathOr', () => {
// This first test fails because the value of a.b is 2 (type 'Number')
// and is a different type to the default value 'N/A' (type 'String')
expect(rambdaPathOr('N/A', ['a', 'b'], { a: { b: 2 } })).toBe(2)
expect(rambdaPathOr('N/A', ['a', 'b'], { c: { b: 2 } })).toBe('N/A')
})
An alternative implementation of rambdaPathOr
from the post above that does work:
const rambdaPathOr = curry((defaultValue, inputPath, inputObject) => {
const inputValue = path(inputPath, inputObject)
return type(inputValue) === 'Undefined' ? defaultValue : inputValue
})
...I'm still a little new to the 'data last' function design, so perhaps the above function can be written more elegantly using compose
, pipe
, ifElse
, equals
etc?
Thank you for bringing up this issue.
I think that current defautTo
implementation brings a bit more safety in some edge cases:
const timeLimit = R.defaultTo(
5,
numberOrUndefined/1000
)
I like that I have this option even if it is not the most common case. I created this behavior because I needed in practice and I still find it useful.
Anyway, your proposal for pathOr
method seems really nice and we'll add it to the API. The remaining question is do you want to do PR or you are leaving this to me?
Fair enough re. defaultTo
—I can certainly appreciate it's use case.
Reason I posted this issue is because it is a common design pattern for an options
object to be passed to a function/constructor where certain key values can have multiple types (typically 2).
One such example might be a key value on an options
object that can either be a function
or a falsy value like false
or null
.
I have been working on a package called ig-api
that uses rambda
(thank you and kudos btw, it's really nice). I make use of an options
object that has a couple of key values that can either be transform functions
or set to false
to prevent any transformations from happening. You can read the docs here.
By default, some built-in transform functions are used unless the user specifies their own or chooses to disable them by passing false
. In this use case, the default value is typically a function
, but a value of false
can be provided and is totally valid. With defaultsTo
this does not work since false
is obviously not the same type as the default function
value.
Anyway, I digress. With regards to pathOr
, I think it would be better if you could implement this please. While my implementation below certainly works and passes the tests, I expect there might be a better way of writing it using rambda
's internals? If not, then please go ahead and simple add my implementation:
const pathOr = curry((defaultValue, inputPath, inputObject) => {
const inputValue = path(inputPath, inputObject)
return type(inputValue) === 'Undefined' ? defaultValue : inputValue
})
I see your use case and I am convinced that defaultTo
should follow original spec, which is if second argument is one of null, undefined or NaN then return the first argument.
As for the current implementation with type checking - it will be moved to Rambdax library as method R.default
I will add the new version of defaultTo
and pathOr
till the end of the way.
Btw, your library looks very promising - keep the good work :)
If you let me know I will add it as a showcase for Rambda.
null
and NaN
should not be considered the same as undefined
. null
and NaN
are actual values, undefined
is no value.
pathOr
should only default to defaultValue
when the type
is 'Undefined'. This is really important. This is how lodash's get
function works.
Might I suggest you implement the function I posted above as pathOr
to align with ramda
's API (with the nice addition of being able to specify the path using dot notation like lodash—nice one 😄)
...and with regards to defaultTo
, perhaps provide 2 versions—one that does the type checking equality and one that doesn't. That way pathOr
could use defaultTo
internally:
// this is how I think defaultTo should work (could rename to 'defaultOr' perhaps?)
const defaultOr = curry((defaultValue, inputValue) => {
return type(inputValue) === 'Undefined' ? defaultValue : inputValue
})
// then you could have your type equality function
const defaultTypeOr = curry((defaultValue, inputValue) => {
return type(inputValue) === type(defaultValue) ? inputValue : defaultValue
})
// pathOr could then use defaultOr
const pathOr = curry((defaultValue, inputPath, inputObject) => {
return defaultOr(defaultValue, path(inputPath, inputObject))
})
// then you could also have
const pathTypeOr = curry((defaultValue, inputPath, inputObject) => {
return defaultTypeOr(defaultValue, path(inputPath, inputObject))
})
Ignore what I posted above re. null
and NaN
not being counted as value types for defaulting to a value...I now see where you got you logic for defaultTo
from: ramda
. Apologies!
http://ramdajs.com/docs/#defaultTo
I also see where the name comes from, so also ignore my suggestion to rename defaultTo
to defaultOr
. You should stick as close to ramda
as possible IMO :)
Yes, I accept your recommendation to stay as close to Ramda API as possible.
Before there were more differences like that R.map/filter
didn't work with objects. Also there were missing simple but valuable APIs as R.ifElse
, R.isNil
, R.identity
, ...
Now Rambda
feel a bit more mature with this fixes and it is also now tree-shaking
-able.
You can see below bundlephobia.com comparison between Rambda
and Ramda
...so to follow up, here is my final proposal:
const defaultTo = curry((defaultValue, inputValue) => {
const inputType = type(inputValue)
const isInputNaN = inputType === 'NaN'
const isInputNull = inputType === 'Null'
const isInputUndefined = inputType === 'Undefined'
const isInputFalsy = isInputNaN || isInputNull || isInputUndefined
return isInputFalsy ? defaultValue : inputValue
})
const typeDefaultTo = curry((defaultValue, inputValue) => {
return type(inputValue) === type(defaultValue) ? inputValue : defaultValue
})
const pathOr = curry((defaultValue, inputPath, inputObject) => {
return defaultTo(defaultValue, path(inputPath, inputObject))
})
const typePathOr = curry((defaultValue, inputPath, inputObject) => {
return typeDefaultTo(defaultValue, path(inputPath, inputObject))
})
In the context of not adding additional API, I think it is better to pass an additional argument, which will enable type checking.
So for type checking R.defaultTo
will look like R.defaultTo(1, foo, true)
. It doesn't look very nice, but it is an option.
The other option is to add typeDefaultTo
and typePathOr
to Rambdax
. It doesn't look very nice, I'll give you that.
Let me know what you think about it?
defaultTo
should have the same signature as it does in ramda
IMO.
Adding another argument wouldn't work because it doesn't adhere to ramda's "data last" design. With your proposal of R.defaultTo(1, foo, true)
the data is demoted to the second to last argument—so the usefulness of currying diminishes somewhat since you wouldn't actually get the value back until you passed all 3 args. This would make the final arg mandatory as opposed to optional.
Since the function you desire with value type checking isn't apart of ramda
(at least as far as I can tell), I would suggest that you simply create a new function like typeDefaultTo
and add it to rambda
.
I think this is perfectly acceptable and useful to a lot of people. Just because it isn't apart of ramda
doesn't mean it shouldn't be apart of rambda
😃
It's got a pretty small footprint, so it's only going to add a few bytes right?
On a separate note, I've just been looking at the source code of some of your modules and wondered why you weren't using the internal curry
function within other modules? For example, the defaultTo
module could be made a lot smaller by using curry
as I have proposed above.
Up until now R.defaultTo
didn't provide curring, which will change with the next commit.
As for your argumentation to add typeDefaultTo
to Rambda
- it seems like the best solution so I will go with that.
What about typePathOr
? Do you want it also as a new API to Rambda
?
Yer I think that would also be a really useful function to have for sure!
Not sure whether the functions should be named:
typeDefaultTo
typePathOr
or
typedDefaultTo
typedPathOr
...basically type
or typed
—what do you think?
:) I was thinking the whole time that typed
will be better.
So we'll do that then. As I mentioned earlier I will add the methods before the end of the day.
Nice one, very much appreciated!
Closing the issue as version 0.9.1 has now all proposed changes.