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

Does optional fields should be marked null? #5

Closed
lchimaru opened this issue Apr 11, 2019 · 11 comments
Closed

Does optional fields should be marked null? #5

lchimaru opened this issue Apr 11, 2019 · 11 comments

Comments

@lchimaru
Copy link
Collaborator

lchimaru commented Apr 11, 2019

I was wondering what was behind the idea of representing optional fields as type | null? Pure optional fields in types/interfaces are marked with question mark and return undefined if they're not exist.

@lstkz
Copy link
Owner

lstkz commented Apr 12, 2019

Does it cause any issues?

User.create({
  title: 'a',
  author: 1,
});

new User({
  title: 'a',
  author: 1,
}).save();

in both cases, arguments are any.

From @types/mongoose

    create(...docs: any[]): Promise<T>;

Not sure if we should override it.

Currently, static typing is only useful when we type user.<prop>, and type | null doesn't cause any problems. It's possible to map it to type?, but type definition will be more complex if you hover on it in VS Code.

@lchimaru
Copy link
Collaborator Author

lchimaru commented Apr 12, 2019

I see another use case for static typing:

type ExtractInterfaceFromDoc<T> = Pick<T, Exclude<keyof T, keyof Document>>;

const exampleSchema = createSchema({
  firstName: Type.string(),
  lastName: Type.string(),
  middleName: Type.optionalString()
});

export const exampleModel = typedModel('example', exampleSchema);
export type IExampleDoc = ExtractDoc<typeof exampleModel>;
export type IExample = ExtractInterfaceFromDoc<IExampleDoc>;

const exampleInstance: IExample = {
  firstName: 'first',
  lastName: 'last'
}

Creating exampleInstance variable with IExample type return error, due to missing middleName property. Is there any way to mark properties with values Type.optionalAnything() as optional? (with question marks)?

@lstkz
Copy link
Owner

lstkz commented Apr 12, 2019

Ok, I see. This is a valid situation.

Added in 0.0.13 ExtractProps.
ExtractDoc, and ExtractProps require a schema instead of modal.

See here full example https://github.com/BetterCallSky/ts-mongoose#extracting-document-type

@lchimaru
Copy link
Collaborator Author

lchimaru commented Apr 12, 2019

Woah! Amazing :) There is one edge case left, if there are nested objects with optional properties, like this:

const exampleSchema = createSchema({
  firstName: Type.string(),
  lastName: Type.string(),
  profile: Type.object().of({
    picture: Type.string(),
    phone: Type.optionalString()
  })
});

ExtractProps will still receive

{
  firstName: string;
  lastName: string;
  profile: {
      picture: string;
      phone: string | null | undefined;
  };
} & {}

instead of this (according to current way of working):

{
  firstName: string;
  lastName: string;
} & {
  profile: {
    picture: string;
    phone?: string | null | undefined;
  }
}

@lstkz
Copy link
Owner

lstkz commented Apr 12, 2019

Can you check 0.0.14? There are more null/undefined fixes.

@lchimaru
Copy link
Collaborator Author

Now it is working fine, no error return on creating following typed variable:

const example: IExampleProps = {
  firstName: 'a',
  lastName: 'b',
  profile: {
    picture: 'a'
  }
}

but I'm not sure about type information provided on hover in vscode, because in previous version optional fields where marked with question mark, and now they are not. It doesn't matter if this property is nested or not.

@lstkz
Copy link
Owner

lstkz commented Apr 12, 2019

VSCode sometimes doesn't show final type in the popup.
It's related to microsoft/TypeScript#22575

@lchimaru
Copy link
Collaborator Author

One more question, according to topic title: any reason for putting null to parameter types? Shouldn't be just type for required parameters and type | undefined for optional ones?

@lstkz
Copy link
Owner

lstkz commented Apr 19, 2019

Does it cause any problems during development?

if the type is optional, and you want to remove the value, you can do:

user.phone = null;
user.save();

Setting to undefined sometimes is not working as expected in some cases (but I am not sure).

user.phone = undefined;

@lchimaru
Copy link
Collaborator Author

lchimaru commented Apr 19, 2019

Is it better to use typical object approach in this case?

delete user.phone;
user.save();

@lchimaru
Copy link
Collaborator Author

Optional fields are not marked as null from version 0.0.20.

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

2 participants