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(server-functions): omitted any from fieldErrors #6720

Closed
wants to merge 9 commits into from

Conversation

tzdesign
Copy link
Contributor

Typescript crashes because any could be infinite

fix #6719

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

See

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

Typescript crashes because any could be infinite

fix QwikDev#6719
@tzdesign tzdesign requested review from a team as code owners July 24, 2024 11:14
Copy link

changeset-bot bot commented Jul 24, 2024

⚠️ No Changeset found

Latest commit: 8d39078

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Jul 24, 2024

‼️ Deploy request for qwik-insights rejected.

Name Link
🔨 Latest commit 271baaf

@tzdesign
Copy link
Contributor Author

@gioboa @shairez Looks like I've tripped myself up. We sometimes use any because we have multi-step forms. Typescript unfortunately can't handle it. Showing: Type instantiation is excessively deep and possibly infinite.

I rewrote it so that the type never ends up in fieldErrors because Zod ignores any anyway.

I hope one of you can write typescript a little better than I can. I have patched it with the small change to
{} extends T[K] which excludes all any types. Let's see if it still works like this in later Typescript versions.

@tzdesign tzdesign changed the title fix(server-functions): omitted any from zod schemas fix(server-functions): omitted any from fieldErrors Jul 24, 2024
Copy link

pkg-pr-new bot commented Jul 24, 2024

commit: 0e16913

npm i https://pkg.pr.new/@builder.io/qwik@6720
npm i https://pkg.pr.new/@builder.io/qwik-city@6720
npm i https://pkg.pr.new/eslint-plugin-qwik@6720
npm i https://pkg.pr.new/create-qwik@6720

Open in Stackblitz

@tzdesign tzdesign requested a review from wmertens August 1, 2024 07:58
@tzdesign
Copy link
Contributor Author

tzdesign commented Aug 2, 2024

@fabian-hiller just to let you know the ValidatorErrorKeyDotNotation wasn't capturing Record types. Added IsAny to the types.ts in the runtime and added tests.

Maybe you can catch up with me some time talking about rewriting this a bit more.
There are still edge-cases for arrays in my concept, but it's better than having the top-level keys with flatten rod errors.

@fabian-hiller
Copy link
Contributor

Have you seen my PR #6752?

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM

@wmertens
Copy link
Member

wmertens commented Aug 2, 2024

@fabian-hiller I'd like to merge this already, it will cause your pr to get merge conflicts though. Ok?

: `${Prefix}${K}`;
}[keyof T & string]
: never;
export type IsAny<T> = 0 extends T & 1 ? true : false;
Copy link
Contributor

@fabian-hiller fabian-hiller Aug 2, 2024

Choose a reason for hiding this comment

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

I think the IsAny type is just a internal util type similar to the Prettify type above and should not be exported.

@fabian-hiller
Copy link
Contributor

fabian-hiller commented Aug 2, 2024

@wmertens and @tzdesign, this PR contains the same any fix as #6752. To avoid merge conflicts, it might be easier to just move the server-functions.unit.ts file to my PR and add Tobi as a co-author.

@tzdesign
Copy link
Contributor Author

tzdesign commented Aug 2, 2024

@fabian-hiller I'm fine with it, but I copied your type and your type includes any as type string. So I added a bit more logic.

I'm fine merging the prs together.

@fabian-hiller
Copy link
Contributor

fabian-hiller commented Aug 2, 2024

Ahh... So you recommend that I should also copy your entire ValidatorErrorKeyDotNotation type?

@wmertens
Copy link
Member

wmertens commented Aug 2, 2024

Ahh... So you recommend that I should also copy your entire ValidatorErrorKeyDotNotation type?

I think that's a good idea

fabian-hiller added a commit to fabian-hiller/qwik that referenced this pull request Aug 2, 2024
>
>
Co-authored-by: Tobi <tzdesign@users.noreply.github.com>
fabian-hiller added a commit to fabian-hiller/qwik that referenced this pull request Aug 2, 2024
Co-authored-by: Tobi <tzdesign@users.noreply.github.com>
@fabian-hiller
Copy link
Contributor

Done

@tzdesign
Copy link
Contributor Author

tzdesign commented Aug 2, 2024

@fabian-hiller perfect!

Thank you guys.
image

@tzdesign tzdesign closed this Aug 2, 2024
wmertens pushed a commit that referenced this pull request Sep 8, 2024
…#6752)

* feat: add valibot$ validator and fix types of zod$ implementation

* fix: brand typed data validators

* fix: update API documentation of Qwik City

* feat: copy code from PR #6720 to merge PRs

Co-authored-by: Tobi <tzdesign@users.noreply.github.com>

* fix: document change with changeset

* chore: change version declaration of Valibot dependency

* chore: change version declaration of Valibot dependency

* fix: change text of changeset

* chore: Add alpha preview warning to valibot$ API

---------

Co-authored-by: Tobi <tzdesign@users.noreply.github.com>
wmertens pushed a commit that referenced this pull request Sep 24, 2024
…#6752)

* feat: add valibot$ validator and fix types of zod$ implementation

* fix: brand typed data validators

* fix: update API documentation of Qwik City

* feat: copy code from PR #6720 to merge PRs

Co-authored-by: Tobi <tzdesign@users.noreply.github.com>

* fix: document change with changeset

* chore: change version declaration of Valibot dependency

* chore: change version declaration of Valibot dependency

* fix: change text of changeset

* chore: Add alpha preview warning to valibot$ API

---------

Co-authored-by: Tobi <tzdesign@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[🐞] fieldErrors - Type instantiation is excessively deep and possibly infinite.
3 participants