Skip to content

fix: simplify null and undefined checks in get function #426

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

OgabekYuldoshev
Copy link

@OgabekYuldoshev OgabekYuldoshev commented Aug 5, 2025

Summary

This PR fixes the get function to properly handle null values by returning the default value when the final resolved value is null. Previously, the function only checked for undefined at the end, allowing null values to be returned even when a default value was provided.

Changes:

  • Updated the final return condition to check for both undefined and null values
  • Ensures consistent behavior where both null and undefined trigger default value fallback
  • Maintains backward compatibility for all existing use cases

This improves the robustness of the utility function and provides more predictable behavior when dealing with nullable data structures.

Related issue, if any:

For any code change,

  • Related documentation has been updated, if needed
  • Related tests have been added or updated, if needed
  • Related benchmarks have been added or updated, if needed
  • Release notes in next-minor.md or next-major.md have been added, if needed

Does this PR introduce a breaking change?

No

This change maintains backward compatibility. The only behavioral change is that null values at the end of a path resolution will now return the default value instead of null, which aligns with the expected behavior of most developers using this utility function.

Bundle impact

Status File Size 1 Difference
M src/object/get.ts 167 -23 (-12%)

Footnotes

  1. Function size includes the import dependencies of the function.

@radashi-bot
Copy link

radashi-bot commented Aug 5, 2025

Benchmark Results

Name Current Baseline Change
get ▶︎ with simple path 2,739,464.13 ops/sec ±0.14% 2,837,296.5 ops/sec ±0.19% 🔗 🐢 -3.45%
get ▶︎ with array index path 1,250,012.87 ops/sec ±0.92% 1,369,164.74 ops/sec ±0.65% 🔗 🐢 -8.7%
get ▶︎ with default value 1,674,100.68 ops/sec ±0.18% 1,788,207.39 ops/sec ±0.16% 🔗 🐢 -6.38%
get ▶︎ with undefined nested path and default value 828,806.32 ops/sec ±0.51% 916,839.99 ops/sec ±0.14% 🔗 🐢 -9.6%

Performance regressions of 30% or more should be investigated, unless they were anticipated. Smaller regressions may be due to normal variability, as we don't use dedicated CI infrastructure.

Copy link
Member

@aleclarson aleclarson left a comment

Choose a reason for hiding this comment

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

I agree with this change. It makes get behave similarly to a ?? b.

This is a behavior change, so a test needs to be updated or added such that it will fail when using the previous implementation. This behavior should also be documented.

OgabekYuldoshev and others added 2 commits August 11, 2025 11:22
- Updated get() to return defaultValue when the resolved value is null, matching behavior for undefined.
- Simplified check to use `current == null` for both null and undefined.
- Added unit tests to cover:
  * Returning defaultValue when final value is null
  * Preserving falsy but non-nullish values (0, '', false)
- Updated documentation to note null handling and show examples.
@aleclarson
Copy link
Member

On second thought, maybe get shouldn't have a defaultValue parameter at all?

Can't you just do this?

_.get(obj, key) ?? defaultValue

@OgabekYuldoshev
Copy link
Author

I think we should keep the defaultValue parameter for consistency with other Radashi utilities, better readability in chained calls, and to avoid extra wrapping with ??. This change focuses on improving nullish handling without removing that convenience.

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.

3 participants