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

Check property existance before accessing it #243

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Mojashi
Copy link

@Mojashi Mojashi commented Mar 1, 2024

Closes #213

As described in the issue above,

export interface SomeInterface {
  prop: { key1: string } | { key2: string };
}

in cases like the one shown, the following code is generated, which violates the noImplicitAny rule when accessing key1.

 typeof typedObj["prop"]["key1"] === "string"
  • I have fixed this issue by ensuring that the property's existence is always checked before accessing it.
  • For optional properties, I have implemented the condition to be either "nonexistent" or "existent and of the correct type".
    • It's important to note that a: number|undefined and a?: number are different.

It's possible to implement a special case only for scenarios where a union is used, but I believe this method to be simpler.

@Mojashi
Copy link
Author

Mojashi commented Mar 1, 2024

@rhys-vdw This change will require modifications to many tests (63 in total), so I would like to confirm that this approach is acceptable before proceeding with those corrections. I have made corrections and implementations for several key tests.

typeof typedObj === "object" ||
typeof typedObj === "function") &&
( !("foo" in typedObj) ||
"foo" in typedObj &&
Copy link
Owner

Choose a reason for hiding this comment

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

!("foo" in typedObj) || "foo" in typedObj && // ...

This check seems redundant, given that the previous code handled this case just fine.

@rhys-vdw
Copy link
Owner

rhys-vdw commented Mar 7, 2024

Hey @Mojashi, not sure if this change to existing tests makes sense. They are now much more complicated but they were passing before. Is this avoidable?

I have fixed this issue by ensuring that the property's existence is always checked before accessing it.

Hm. I wonder if we should be avoiding typedObject entirely and just casting to any. The typing helps to sanity check the output.

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.

Type guards not compatible with noImplicitAny
2 participants