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(ts): set payload to unknown value type #272

Merged
merged 1 commit into from
Sep 10, 2023

Conversation

kentcdodds
Copy link
Contributor

The Jsonify utility used by Remix will make a Record<string, any> disappear in the type: remix-run/remix#7246

But considering this further, "unknown" is a safer option for this type anyway because it avoids issues with accidentally accessing the values off the submission payload without first checking them. So I do think this change should be made.

kentcdodds added a commit to epicweb-dev/web-forms that referenced this pull request Aug 24, 2023
edmundhung/conform#272

Until that is merged, we need to manually update it in node_modules
@MichaelDeBoey
Copy link

MichaelDeBoey commented Aug 24, 2023

The TS team also suggests to use unknown instead of any: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#any

kentcdodds added a commit to epicweb-dev/data-modeling that referenced this pull request Aug 25, 2023
@danestves
Copy link

Can confirm that this fix works with the new v2 of Remix

Before After

Copy link
Owner

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

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

Thank you!

@edmundhung edmundhung changed the base branch from main to next August 29, 2023 20:54
The Jsonify utility used by Remix will make a `Record<string, any>` disappear in the type: remix-run/remix#7246

But considering this further, "unknown" is a safer option for this type anyway because it avoids issues with accidentally accessing the values off the submission payload without first checking them. So I do think this change should be made.
@danestves
Copy link

Any progress on this? 😬

@edmundhung
Copy link
Owner

I will update the remix fixture to v2 (pre) this weekend to verify the change and make a release.

I was betting the issue to be resolved upstream soon so I don't need to bump the minor version now (This is a breaking change unfortunately). But it looks like type-fest is still waiting for a proper fix and the issue on remix has been de-prioritized as well. 😅

@edmundhung edmundhung merged commit d5adc47 into edmundhung:next Sep 10, 2023
1 check passed
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.

4 participants