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

protect against any more #66

Closed
wants to merge 7 commits into from
Closed

protect against any more #66

wants to merge 7 commits into from

Conversation

mmkal
Copy link
Owner

@mmkal mmkal commented Mar 19, 2024

@aryaemami59 @mrazauskas

Proposal for protecting against any in a fairly simple way. This might break some people's tests, but I imagine 99% are in cases where an any has unexpectedly snuck in.

Related: this comment

@mmkal mmkal mentioned this pull request Mar 19, 2024
1 task
@mrazauskas
Copy link
Contributor

This looks interesting. I checked out this branch quickly with:

expectTypeOf<any>().toEqualTypeOf<{a: number}>() // not allowed
expectTypeOf<any>().toMatchTypeOf<{a: number}>() // not allowed

expectTypeOf<{a: any}>().toEqualTypeOf<{a: number}>() // fail
expectTypeOf<{a: any}>().toMatchTypeOf<{a: number}>() // pass !!!

expectTypeOf<any[]>().toEqualTypeOf<number[]>() // fail
expectTypeOf<any[]>().toMatchTypeOf<number[]>() // pass !!!

Nested anys are not handled. Hm.. Feels like current implementation is more predictable. It just needs to be documented.

Please understand me right, I do admire all the effort which went into writing this library. My brain could hardly come up with something similar. All I can contribute are these tests for edge cases. No ideas how to handle these anys using generic types.

@mmkal
Copy link
Owner Author

mmkal commented Mar 21, 2024

Nested anys are not handled. Hm.. Feels like current implementation is more predictable. It just needs to be documented.

Yes, this looking for nested anys would be straying too far from what vanilla typescript does, IMO. The cases you gave above seem right to me. toMatchTypeOf is meant to be a loose check. any is unfortunately more loose than most would want even for an officially-loose check. But there isn't currently an option for something in between toEqualTypeOf and toMatchTypeOf, which is maybe what you want.

I have thought about adding an assertion like this, which might be a useful "did I do anything stupid, anywhere" check:

const myValue = {
  deeply: {
    nested: {
      property: {
        value: JSON.parse('{}'), // whoops, this is an any
      },
    },
  },
}

expectTypeOf(myValue).notToHaveAnyAnys()

Which would fail with some helpful error containing a message along the lines of 'any' found at path 'deeply.nested.property.value'.

Please understand me right, I do admire all the effort which went into writing this library. My brain could hardly come up with something similar. All I can contribute are these tests for edge cases. No ideas how to handle these anys using generic types.

Thank you - it's helpful!

@aryaemami59
Copy link
Collaborator

@mmkal This is good, however I think this should probably be opt in, and since it's checking nested fields, TypeScript might not be able to handle it well especially on lower versions. Here is an idea, we can implement this to sort of mirror .branded. So we can have .branded.toEqualTypeOf() and .strict.toMatchTypeOf(). This way we avoid a breaking change and implement a new feature that is syntactically similar to an already existing feature.

@aryaemami59 aryaemami59 added the New feature New feature or request label Mar 22, 2024
@mmkal
Copy link
Owner Author

mmkal commented Jun 10, 2024

Having let this sit for a while, I'm inclined to not do it. Esp since the feedback is "don't do this, it's inconsistent" or "make it opt in, it's inconsistent". I'd like to avoid too many different ways to do things, so I think we should just accept that anys can trip up toMatchTypeOf, just like they can trip up pretty much everything in the TypeScript world.

Maybe for a future major version (v2 or beyond).

@mmkal mmkal closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants