Unexpected behaviour when syncing to object with hashid-based keys

[liamtlr]

Hello Dave! Many thanks for your work on this project. It's a real help and much appreciated! Thought I would bring this to your attention in case you consider it a bug and/or warranting attention.

Describe the bug

When a component syncs to a particular key/value pair (using :notation), if the key is a zero-leading string consisting solely of numerical values, the new value is inserted as a new key value pair (with the leading zero trimmed), as opposed to updating the existing key/value pair.

Expected behaviour

The existing key/value pair is updated.

Additional context

Believe this behaviour is arising from the key being mutated when it is passed to parseInt here. This is not an issue for other numerical strings as they are effectively unchanged in the change to Number.

This behaviour can be replicated on the latest version as per the codesandbox below. It may well be that the implementation giving rise to this is not an advised pattern, appreciate any thoughts you may have to that end.

Code example

https://codesandbox.io/s/cranky-wildflower-fl85t?file=/src/sample.test.js

[liamtlr]

Appreciate you have many draws on your time. If no action is to be taken here, or it will be a long time in coming, please do let me know and I will address this in my project in another way. Thanks once again!

[davestewart]

Hey Liam,

Thanks for the kind words!

So that code is basically designed to handle paths where you want to index something as an array.

It's pretty rare – I would consider it bad design – in JS to use a numerical key for object properties.

The regex in that code /^\d+$/ checks that the key is entirely numbers which would be a pretty safe bet for indices. But what would happen for example if you indexed 00 to 10 – but checking for leading 0s, vs no 0s? 00 to 09 would be strings, and 10 would be a number. So moving the logic into Pathify would be a bad idea. A number is not always a number and sometimes a string 😬

My guess is that in this situation, you should just prefix whatever value it is with a string, such as "item".

Do you have a "real life" example that is not working for you?

Perhaps you can provide some code / context and we can work it out 🙂

[liamtlr]

Quick response! I assumed that to be the case in terms of the reason for the check.

In terms of code the codesandbox replication is basically the exact structure we're using (a set of components individually syncing their local data to a central store-based object). Context-wise, we are keying on a hashed ID corresponding to a related backend entity. Agreed a numeric object key is undesirable, alas we cannot seem to rule out these from being in the offending format. We have a flaky unit test which fails when this is the case (more often than you'd imagine) 😫 .

Would checking the format of the target (regular object vs. array vs. whatever) as opposed to the path be an option? Otherwise as you've suggested there are loads of solutions to this. Just wanted to raise it in case you wanted to support other fools like me!

[davestewart]

Ha! Well, in the immortal words of Steve Jobs, "Stay Hungry. Stay Foolish" 😛

Would checking the format of the target (regular object vs. array vs. whatever) as opposed to the path be an option?

You know what... I don't mind that idea.

Let me see if that's a 5-minute fix and a 5-minute push. If so, happy to do it!

Otherwise, you could build your own syncThing() helper to return a computed property object which addresses the store directly.

Not doing anything now, so give me 10...

[davestewart]

We're basically looking at this, right?

    if (Array.isArray(obj) && isIndex) {
      key = parseInt(key)
    }

maybe a bit more for:

      if (create) {
        obj[key] = isIndex ? [] : {}
      }

[davestewart]

Unfortunately, this lib does not have tests (glaring hole) so do me a favour and replace the setValue function in the node_modules/vuex-pathify folder (you will need to experiment which of the files you are using) with this one and see if it works:

export function setValue (obj, path, value, create = false) {
  const keys = getKeys(path)
  return keys.reduce((obj, key, index)  => {
    // early return if no object
    if (!obj) {
      return false
    }

    // convert key to index if key is numeric and obj is an array
    const isIndex = /^\d+$/.test(key)
    if (Array.isArray(obj) && isIndex) {
      key = parseInt(key)
    }

    // if we're at the end of the path, set the value
    if (index === keys.length - 1) {
      obj[key] = value
      return true
    }

    // if the target property doesn't exist, create one, or cancel
    else if (!isObject(obj[key]) || !(key in obj)) {
      if (create) {
        obj[key] = isIndex ? [] : {}
      } else {
        return false
      }
    }

    // otherwise, derive the next key
    return obj[key]
  }, obj)
}

I ran some basic code in Quokka and this seems to work.

EDIT: forgot, we do have tests!

LMK

[davestewart]

OK, this is cool; actually spotted a bug in the creation part of the function, which should be determining whether to create an object or array based on the following key, not the current key.

Final function:

/**
 * Tests whether a string is numeric
 *
 * @param   {string|number}   value   The value to be assessed
 * @returns {boolean}
 */
export function isNumeric (value) {
  return typeof value === 'number' || /^\d+$/.test(value)
}

export function setValue (obj, path, value, create = false) {
  const keys = getKeys(path)
  return keys.reduce((obj, key, index) => {
    // early return if no object
    if (!obj) {
      return false
    }

    // convert key to index if obj is an array and key is numeric
    if (Array.isArray(obj) && isNumeric(key)) {
      key = parseInt(key)
    }

    // if we're at the end of the path, set the value
    if (index === keys.length - 1) {
      obj[key] = value
      return true
    }

    // if the target property doesn't exist, the final option is to create one
    else if (!isObject(obj[key]) || !(key in obj)) {
      if (create) {
        // create object or array, depending on next key
        obj[key] = isNumeric(keys[index + 1])
          ? []
          : {}
      }
      else {
        return false
      }
    }

    // if we get here, return the target property
    return obj[key]
  }, obj)
}

I think this should all work for your purposes, but still LMK and then I will push and release a new version.

[davestewart]

Published as 1.4.5: https://www.npmjs.com/package/vuex-pathify/v/1.4.5

[liamtlr]

This is spot on - can confirm this addresses my edge case. Thanks a bunch for the quick turnaround time - super helpful!

[davestewart]

No problem! Thanks for flagging. It also lead me to fix a crappy oversight / bug so 🥳