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

feat: support escaped paths in field name #551

Closed

Conversation

chrisirhc
Copy link
Contributor

@chrisirhc chrisirhc commented Jan 4, 2024

Attempts to address #532 .

This is a sketch implementation for feedback, using lodash's internals for get.
Happy to roll a custom implementation but I thought of using an existing battle-tested implementation.

This implementation handles some of the cases in https://github.com/lodash/lodash/blob/main/test/toPath.spec.js .

Copy link

codesandbox-ci bot commented Jan 4, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@chrisirhc chrisirhc changed the title feat: support escaped paths feat: support escaped paths in field name Jan 4, 2024
Comment on lines 148 to 156
// key difference from lodash string to path algoritm
if ((/^\d+$/).test(key)) {
key = parseInt(key, 10);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main modification I made based on the previous logic in makePathArray.

@crutchcorn
Copy link
Member

This looks good overall! The only concern I have now is typings. We'll want a way to merge the types from string[] to string. This can be done like so:

https://github.com/crutchcorn/ts-util-helpers/blob/main/src/concat-string.ts#L1-L7

But then the question becomes "how do we get TName inference working for this?

@chrisirhc
Copy link
Contributor Author

chrisirhc commented Jan 11, 2024

This looks good overall! The only concern I have now is typings. We'll want a way to merge the types from string[] to string.

Not sure I get this part of your comment. Where are you referring to the string[] type? Is it the makePathArray?

But then the question becomes "how do we get TName inference working for this?

Just to make sure I get what you're referring to. You're referring to making sure that the DeepKeys

export type DeepKeys<T, TDepth extends any[] = []> = TDepth['length'] extends 5
can work for the [''] syntax, right? I'll look in this.

@crutchcorn
Copy link
Member

I wouldn't modify DeepKeys. I would modify the instance where TName is being passed and introduce a new utility type of some kind.

@chrisirhc chrisirhc force-pushed the chrisirhc/support-escaped-paths branch from 88ce14c to 5463b69 Compare January 22, 2024 04:16
@chrisirhc chrisirhc force-pushed the chrisirhc/support-escaped-paths branch from 5463b69 to 26b7d61 Compare January 22, 2024 04:17
@chrisirhc chrisirhc force-pushed the chrisirhc/support-escaped-paths branch from 66897bc to d778e69 Compare January 22, 2024 05:19
@chrisirhc
Copy link
Contributor Author

Added spec updates and also utilities as discussed. Ready for comment/review.

@codecov-commenter
Copy link

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (08ad74d) 87.78% compared to head (92bc078) 87.65%.

Files Patch % Lines
packages/form-core/src/utils.ts 89.47% 2 Missing ⚠️
packages/react-form/src/useField.tsx 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
- Coverage   87.78%   87.65%   -0.14%     
==========================================
  Files          31       31              
  Lines         819      826       +7     
  Branches      184      187       +3     
==========================================
+ Hits          719      724       +5     
- Misses         95       97       +2     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crutchcorn
Copy link
Member

crutchcorn commented Mar 23, 2024

Hey, I am super sorry - but we've made drastic changes to the internals of these files. It's honestly better to close out this PR and revisit them. I think our types support this now, but our logic certainly doesn't.

Let's track the progress of this in: #639

Feel free to open a new PR with the conflicts resolved if you're up for the task. I'll make sure we review it much sooner - I apologize for the crossed wires here. Arrays were in a really bad spot anyway

@crutchcorn crutchcorn closed this Mar 23, 2024
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