fix(isDeepKey): improve deep key detection to match strict patterns#1621
fix(isDeepKey): improve deep key detection to match strict patterns#1621
Conversation
- Enhance isDeepKey to return false for invalid patterns like 'a.', '.a', 'a[b', etc., ensuring only valid deep keys are detected.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…dified or added by mistake
There was a problem hiding this comment.
Pull request overview
This PR tightens deep-key detection in the compat layer so functions like get/has/unset only treat syntactically valid deep-path strings as deep keys (and otherwise treat the string as a literal property key).
Changes:
- Refines
isDeepKeyto reject several previously-accepted invalid patterns. - Expands
isDeepKeyunit tests to cover additional valid/invalid inputs. - Adds a null-guard in
findIndex’s object-shorthand handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/compat/array/findIndex.ts |
Adds explicit handling for doesMatch === null in the object-shorthand branch. |
src/compat/_internal/isDeepKey.ts |
Reworks deep-key detection logic for dot/bracket patterns. |
src/compat/_internal/isDeepKey.spec.ts |
Adds tests for additional invalid patterns and a few more valid cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(isDeepKey('.a')).toBe(false); | ||
| expect(isDeepKey('a[b')).toBe(false); | ||
| expect(isDeepKey('a]b]')).toBe(false); | ||
| expect(isDeepKey('a][b')).toBe(false); |
There was a problem hiding this comment.
The updated tests cover a handful of invalid patterns, but they don't cover several false positives the new implementation still allows (e.g. a..b, a.b., a[b][c, a[b]], a[b]c, a.b[). Adding cases like these would prevent regressions and ensure the “strict pattern” behavior described in the PR.
| expect(isDeepKey('a][b')).toBe(false); | |
| expect(isDeepKey('a][b')).toBe(false); | |
| expect(isDeepKey('a..b')).toBe(false); | |
| expect(isDeepKey('a.b.')).toBe(false); | |
| expect(isDeepKey('a[b][c')).toBe(false); | |
| expect(isDeepKey('a[b]]')).toBe(false); | |
| expect(isDeepKey('a[b]c')).toBe(false); | |
| expect(isDeepKey('a.b[')).toBe(false); |
| if (dotIndex > 0 && dotIndex < key.length - 1) { | ||
| return true; |
There was a problem hiding this comment.
The dot check only looks at the first '.' and only enforces it isn't the first/last character. This still returns true for syntactically invalid dot paths like a..b (empty segment) or a.b. (trailing dot), which contradicts the PR goal of accepting only strict deep-key patterns. Consider validating the full string (e.g., reject consecutive dots / any empty segment) rather than just indexOf('.') bounds checks.
| if (dotIndex > 0 && dotIndex < key.length - 1) { | |
| return true; | |
| if (dotIndex !== -1) { | |
| const segments = key.split('.'); | |
| if (segments.length > 1 && segments.every(segment => segment.length > 0)) { | |
| return true; | |
| } |
| let leftBracketIndex = key.indexOf('['); | ||
| let rightBracketIndex = key.indexOf(']'); | ||
| if (leftBracketIndex !== -1 && rightBracketIndex !== -1 && leftBracketIndex < rightBracketIndex) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The bracket check only verifies that the first '[' appears before the first ']'. It will still return true for invalid/mismatched bracket patterns like a[b][c (unclosed), a[b]] (extra ']'), a.b[ (unclosed), or a[b]c (missing separator). To meet the strict-pattern intent, this likely needs a small parser/regex that validates balanced bracket segments and disallows stray brackets outside of them.
What
isDeepKeyfunction to strictly detect only valid deep key patterns.isKeyuses the same consistent deep property regex for property path detection.Why
Test
isDeepKey.spec.tsto cover both valid and invalid deep key patterns.isKeyandisDeepKeyare consistent in their detection logic.