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

fix: return a null value for undefined nested keys #4620

Closed
wants to merge 3 commits into from
Closed

fix: return a null value for undefined nested keys #4620

wants to merge 3 commits into from

Conversation

SeanStove
Copy link

Since PR #4407 undefined nested keys throw an error message.
This might be helpful, but does not solve the issue when you have to work with data which looks like

[
  { "firstName": "John", "address": { "street": "Main street 1" } },
  { "firstName": "Jane", "address": null }
]

accessor address.street in this case will throw the error in dev.
(or will break the application later in production in case the data here is not as complete)

So the suggestion from @dennemark in issue #4499 makes sense to me.
This PR will return a null value, if the nested value does not exist.
Optionally, there could also be a console.warn statement with what used to be the error message.

Fixes: #4499

Copy link
Member

@KevinVandy KevinVandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do need to do this, but let's also keep a console.warn with that error message in dev mode

@SeanStove
Copy link
Author

I think we do need to do this, but let's also keep a console.warn with that error message in dev mode

@KevinVandy added the warning message.

@kachun333
Copy link

is it possible for this change to be merged, please?

@SeanStove
Copy link
Author

I noticed the key in result will fail if the object is null, so I've added an additional check for that.

So it should now handle these cases:

{ "firstName": "Jane", "address": { "city": "New York" } }

{ "firstName": "James", "address": null }

{ "firstName": "Jimmy" }

when trying to access address.street

throw new Error(
`"${key}" in deeply nested key "${accessorKey}" returned undefined.`
)
if (result == null || !(key in result)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be "===" triple equals equality check? and actually, wouldn't we want to check for undefined? Why is the result assignment moved to the last line?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used "==" to handle both undefined and null, because you cannot use the in operator on either of them. Unless you want to handle undefined and null differently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SeanStove I'm working on a PR where I'm simply making this change. Wonder if it works in your testing

accessorFn = (originalRow: TData) => {
        let result = originalRow as Record<string, any>

        for (const key of accessorKey.split('.')) {
          result = result?.[key]
          if (process.env.NODE_ENV !== 'production' && result === undefined) {
            console.warn(
              `"${key}" in deeply nested key "${accessorKey}" returned undefined.`
            )
          }
        }

        return result
      }

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KevinVandy Tested and it covers the 3 cases I described above. Thanks for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipped this last week if you hadn't seen that yet

}

result = result[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still be above the if statement?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result assignment is moved to the last line because you cannot read properties of null/undefined (which is covered by the if statement above it).

@KevinVandy
Copy link
Member

This code was good, but cleaned up a little in this another PR. Closing this in favor of #4678

@KevinVandy KevinVandy closed this Jan 26, 2023
@SeanStove SeanStove deleted the fix-undefined-nested-keys branch February 3, 2023 09:55
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

Successfully merging this pull request may close these issues.

[v8] getValue should return a null value for nested keys
3 participants