selfrefactor/rambda

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

bundlephobia

...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.