francescov1 / mongoose-tsgen

A plug-n-play Typescript generator for Mongoose.
102 stars 24 forks source link

Isn't generating optional arrays of Strings correctly #124

Closed wootwoot1234 closed 7 months ago

wootwoot1234 commented 1 year ago
const ReservationSchema = new Schema({
    userIds: {
        type: [String],
        required: false,
        default: undefined
    },
    ...
})

Generates:

    userIds: string[];

But I expect it to be optional, like this:

    userIds?: string[];
francescov1 commented 1 year ago

Hey @wootwoot1234, thanks for flagging this, will get right on it! Should be a quick fix, array fields are usually initialized to an empty array which is why we assume they are required, but I guess we arent handling the case when default: undefined.

wootwoot1234 commented 1 year ago

Awesome, thank you!!

francescov1 commented 1 year ago

Fix is live in v9.2.7 🚀

ChristopherMJ commented 9 months ago

Hi @francescov1, I think there is still an issue with an optional array of Strings. The only way I can find to make the array optional is by making the default undefined. Setting default to an empty array "[]" makes it required, although required is explicitly set to false.

const ReservationSchema = new Schema({ userIds: { type: [String], required: false, default: [] }, ... })

Generates userIds: string[];

But optional output as follows is expected because required is false and a default value is specified. userIds?: string[];

francescov1 commented 9 months ago

Nice catch @ChristopherMJ. Most of the logic around arrays assumes that arrays are usually "required", since Mongoose automatically sets them to an empty array and most people never need to unset it. Curious what your use case is for unsetting it? And if its more common than I think.

I think for most users, the current logic makes things easier since many people leave out the required: true flag on arrays but still expect the array to always exist. I usually opt for correctness, so I'm leaning towards changing it to your suggestion, but it will introduce breaking changes to many current users, so I want to think it through a bit more.

ChristopherMJ commented 9 months ago

Thx for your update @francescov1 . Since my last comment, my data structure has changed so I don't need to use "undefined" to make the array optional. Therefore, I don't have a use case I'm trying to satisfy that would prompt me to request that you change the existing code.

But I would still think that if required is false, that alone would be enough to justify output from tsgen matching the explicit choice in the schema. If you could make this change without breaking functionality for existing users, it would be great. Otherwise, you can probably close this issue again.

francescov1 commented 9 months ago

I like the idea of checking if required is explicitly set to false (rather than any time its not set to true). That will be much less disruptive, and still gives users the functionality if they need it.

ChristopherMJ commented 9 months ago

@francescov1 sounds good!

adrianwiechec-at-optilyz commented 7 months ago

It's probably related, but now for:

import { Schema, model } from "mongoose";

const UserSchema = new Schema({
        path: { type: [String], required: true, default: undefined },
});

export const User = model("User", UserSchema);

I get:

export type User = {
  path?: string[];
  _id: mongoose.Types.ObjectId;
};

where you can clearly see path is optional, even though the schema specifies it as required: true

francescov1 commented 7 months ago

Ah yep, previously was looking at "default: undefined" meaning that the field was optional, since this is usually the way to specify that an array should not be automatically set for a new document. Thanks for pointing this out.

Ill deploy a fix to both bugs above shortly. This library is starting to get quite overcomplicated & messy with all the edge cases, its grown much larger than the initial use cases. Will likely need a rewrite sometime soon.

francescov1 commented 7 months ago

Fixed in v9.2.8!

adrianwiechec-at-optilyz commented 7 months ago

Works splendidly now, thank you very much! :bow: