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

Don't allow explicit undefined fields on unions that don't have an undefined case #56398

Closed
6 tasks done
Tsmith18256 opened this issue Nov 14, 2023 · 6 comments
Closed
6 tasks done

Comments

@Tsmith18256
Copy link

Tsmith18256 commented Nov 14, 2023

πŸ” Search Terms

union, undefined, keys, non-shared, properties

βœ… Viability Checklist

⭐ Suggestion

I think unions should enforce that keys can't be assign undefined if neither half of the union accepts it. If I can't assign an object to either interface, I wouldn't expect to be able to assign it to the union of the 2.

As an example, let's say I have these 2 interfaces:

interface IForumPost {
  id: string;
  title: string;
  description: string;
}

// A new forum post doesn't have an ID yet until it is persisted in the database
interface INewForumPost extends Omit<IForumPost, 'id'> {}

I don't have a scenario where the id field should be allowed to exist, but be undefined. Both of these object declarations will throw an error:

const post: IForumPost = {
  id: undefined, // error, needs to be a string for this interface
  title: 'Hello, world!',
  description: 'This is a forum post'
};

const post: INewForumPost = {
  id: undefined, // error, obj doesn't exist on this interface
  title: 'Hello, world!',
  description: 'This is a forum post'
};

The problem I had is that id can be undefined if I have a union of these 2:

const post: IForumPost | INewForumPost = {
  id: undefined, // TypeScript is fine with this
  title: 'Hello, world!',
  description: 'This is a forum post'
};

The part that had this lead to a bug in my code is that TS then doesn't let me simply do an if (post.id) or if (typeof post.id === 'string') check to determine which side of the union I have (those checks would be safe here), but it makes me do if ('id' in post) check (which is not safe here because it's true for undefined).

πŸ“ƒ Motivating Example

Let's say I have a function that tries to route the changes correctly to either "update" or "create":

const saveForumPost = (post: IForumPost | INewForumPost) => {
  if ('id' in post) {
    updateForumPost(post);
  } else {
    createForumPost(post);
  }
};

And I am creating this post object using a form that works for both editing or creating:

const postFromFormData: IForumPost | INewForumPost = {
  id: isNew ? formData.id : undefined,
  title: formData.title,
  description: formData.description
};

That id assignment is a pretty innocent ternary, but actually should not be accepted. saveForumPost now treats this as an existing entry and attempts to update instead of create. This change would require me to either provide a proper value for id or omit the key completely, leading to safer code.

πŸ’» Use Cases

1. What do you want to use this for?

I would like to use this to ensure stronger type safety in my code.

2. What shortcomings exist with current approaches?

Current approaches allow assigning invalid objects to unions.

3. What workarounds are you using in the meantime?

I have fixed the code, as needed, but not until after my bug was found. The workaround for now is just to be extra careful in this type of situation.

@MartinJohns
Copy link
Contributor

MartinJohns commented Nov 14, 2023

Duplicate of #20863. But even with that you can't really rely on it, because TypeScript is structurally typed, excess properties are generally allowed. You'd need "exact types" to avoid this issue. #12936

This is perfectly valid code:

const obj = { id: 123, title: '', description: '' }
const post: INewForumPost = obj

And wow, you use excessive amounts of words as search terms. I think the idea of @fatcerberus to add a "summary" field is really good (mentioned in another issue).

@fatcerberus
Copy link

FWIW, undefined is a red herring - you will find that the id property can be anything at all in that assignment, for the reasons mentioned above.

@Tsmith18256
Copy link
Author

Oh sorry I totally misread the template for the search terms part, I'll fix that now

@Tsmith18256
Copy link
Author

Okay hopefully that's better

Would it be more reasonable to propose a strict setting that would require adding an undefined or typeof check to these kinds of unions instead of just the in check? For scenarios where the structural typing could cause this

@RyanCavanaugh
Copy link
Member

You can do this today:

interface INewForumPost extends Omit<IForumPost, 'id'> {
	id?: never;
}

@Tsmith18256
Copy link
Author

Oh awesome yeah that does what I need. Looks like I actually want to leave the ? off of that example but I just tested it and it blocks the ternary I had before πŸ˜ƒ I think I'm good to close this as a duplicate then

@Tsmith18256 Tsmith18256 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 14, 2023
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

No branches or pull requests

4 participants