Placeholder doesn't work for converted functions unless explicitly recurried

[daedalus28]

When using convert on functions from lodash fp, they seem to lose their support for using _ as a placeholder. If I explicitly call curry on the result, they regain their placeholder support. Here's a full test case showing a failing test:

import _ from 'lodash/fp'

describe('lodash test', () => {
  it('placeholder for converted functions', () => {
    let mutableUnset = _.unset.convert({ immutable: false })
    let x = {
      a: 1,
      b: 1
    }
    console.log(mutableUnset.placeholder) // <-- Seems to be lodash
    console.log(mutableUnset.placeholder == _) // <-- false

    let unsetInX = mutableUnset(_, x)
    unsetInX('a') // <--- throws `TypeError: unsetInX is not a function`
    console.log(x)
  })
  it('placeholder for explicitly curried converted functions', () => {
    let mutableUnset = _.curryN(2, _.unset.convert({ immutable: false }))
    let x = {
      a: 1,
      b: 1
    }
    console.log(mutableUnset.placeholder) // <-- Seems to be lodash
    console.log(mutableUnset.placeholder == _) // <-- true

    let unsetInX = mutableUnset(_, x)
    unsetInX('a') // <--- works because of the explicit curry above
    console.log(x) // { b: 1}
  })
})

[jdalton]

Hi @daedalus28!

Is there a .placeholder property on the mutableUnset function?

[daedalus28]

Yes, there is. It appears to be a lodash instance, but it is not the same one as what's imported. I'm updating my original post to include the lines I added to test for placeholder:

console.log(mutableUnset.placeholder) // <-- Seems to be lodash
console.log(mutableUnset.placeholder == _) // <-- false

[daedalus28]

In the one with the explicit curry, mutableUnset.placeholder == _ is true

[jdalton]

Can you try using the mutableUnset.placeholder placeholder for the unsetInX assignment.

[daedalus28]

Yup, using mutableUnset.placeholder instead of _ works. It seems like something in the conversion is creating a clone of lodash or something like that - it doesn't seem to be an issue with the function of placeholder itself

[jdalton]

Nice!

This is by design and also why we bolt on the .placeholder prop to methods. Lodash allows creating its world again to avoid cross contamination of state and settings. This is part of that.

[daedalus28]

I don't think anyone would expect that behavior. Why does simply calling convert create an entirely new instance of lodash?

[daedalus28]

In practice, this means every method I convert I'll also have to manually re-curry. If nothing else, converting should optionally allow preserving the placeholder instance.

Also, does that mean every time I call convert on a method, I'm creating a whole new instance of lodash in memory? That seems less than ideal from a performance standpoint.

[jdalton]

lodash itself is converted to lodash/fp so converting methods is a natural fit.
You might be able to avoid currying by simply replacing the .placeholder value.

mutableUnset.placeholder = _

If that works I'll patch fp to automatically do that.

[daedalus28]

Yup, reassigning works:

it('placeholder for converted functions', () => {
    let mutableUnset = _.unset.convert({ immutable: false })
    let x = {
      a: 1,
      b: 1
    }
    mutableUnset.placeholder = _
    console.log(mutableUnset.placeholder == _) // <-- true

    let unsetInX = mutableUnset(_, x)
    unsetInX('a') // <--- Works :D
    console.log(x)
  })

[daedalus28]

I would happily do the PR myself but I'm not sure where the code for convert is - it doesn't seem to be in this repo

[jdalton]

Rock 😋!

Don't worry, our master branch is a mess with v5 prep, I'll patch in an isolated branch.

[daedalus28]

@jdalton not to be annoying, but any idea on an ETA for that fix being published?

[jdalton]

@daedalus28

not to be annoying, but any idea on an ETA for that fix being published?

No ETA yet. Thanks for keeping it on my radar though.

[daedalus28]

@jdalton is it possible to push a 4.17.5 that just adds this in?

[daedalus28]

@jdalton Not to be annoying, but it's been almost 2 months. Any chance of getting the patch released as 4.17.5?

[jdalton]

When we do a backport of webpack's sideEffects flag this will likely get swept up with that. I'm not actively working on v4 right now. My attention is on v5.

[daedalus28]

@jdalton There's been a couple of fix revision releases, but I haven't seen this in yet. Was it just missing in the changelog, or is this still open?

[jdalton]

Ah @daedalus28!

This totally did not get fixed. I missed it 😱
I can prepare a patch to do a minor release by end of day Friday.

[daedalus28]

@jdalton did that happen?

[jdalton]

Annnnd I missed it. (the three day weekend got me).
It's in my email marked as important so I should keep seeing it :)

[daedalus28]

Checking in... any update?

[jdalton]

Patched 2de676f.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.