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

Challenges with working on embedded arrays and typing issues #15041

Closed
1 task done
nikzanda opened this issue Nov 15, 2024 · 5 comments · Fixed by #15085
Closed
1 task done

Challenges with working on embedded arrays and typing issues #15041

nikzanda opened this issue Nov 15, 2024 · 5 comments · Fixed by #15085
Labels
help wanted help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary
Milestone

Comments

@nikzanda
Copy link

Prerequisites

  • I have written a descriptive issue title

Mongoose version

8.6.0

Node.js version

20.10.0

MongoDB version

6.0.2

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

No response

Issue

I find it hard to work with embedded arrays: the documentation is unclear, and I often have to open issues just to understand how to type my models and embedded arrays correctly.

In the reproduction link, you can see the User model with two embedded arrays: hobbies and addresses. In the UserDocumentOverrides type, hobbies are typed with Types.DocumentArray<HobbyInstance>, and addresses are typed as AddressInstance[]. Both approaches have pros and cons: using Types.DocumentArray allows me to push new elements into the array without typing errors, but the other standard array methods are not implemented; using a plain array type like AddressInstance[] implements all array methods, but I cannot use methods like push, unshift, or splice because of the missing properties from the HydratedSubdocument type.

There is also a recently closed issue that suggests using Types.DocumentArray<EmbeddedInstance> instead of EmbeddedInstance[], but this still does not seem to be the correct way to work with arrays as expected.

Below are the examples described above:

User model

interface IUser {
  _id: Types.ObjectId;
  name: string;
  hobbies: IHobby[];
  addresses: IAddress[];
  createdAt: Date;
  updatedAt: Date;
}

type UserDocumentOverrides = {
  hobbies: Types.DocumentArray<HobbyInstance>;
  addresses: AddressInstance[];
};

interface IUserVirtuals {
  id: string;
}

type UserInstance = HydratedDocument<
  IUser,
  IUserVirtuals & UserDocumentOverrides
>;

type UserModelType = Model<
  IUser,
  {},
  UserDocumentOverrides,
  IUserVirtuals,
  UserInstance
>;

const userSchema = new Schema<IUser, UserModelType>(
  {
    name: {
      type: SchemaTypes.String,
      required: true,
      trim: true,
    },
    hobbies: {
      type: [hobbySchema],
      default: [],
    },
    addresses: {
      type: [addressSchema],
      default: [],
    },
  },
  { timestamps: true, optimisticConcurrency: true }
);

userSchema.index({ name: 1 });

Hobby embedded

interface IHobby {
  name: string;
}

type HobbyDocumentOverrides = {};

interface IHobbyVirtuals {}

type HobbyInstance = HydratedSingleSubdocument<
  IHobby,
  IHobbyVirtuals & HobbyDocumentOverrides
>;

type HobbyModelType = Model<
  IHobby,
  {},
  HobbyDocumentOverrides,
  IHobbyVirtuals,
  HobbyInstance
>;

const hobbySchema = new Schema<IHobby, HobbyModelType>(
  {
    name: {
      type: SchemaTypes.String,
      required: true,
      trim: true,
    },
  },
  {
    // toObject: true // does not compile
  }
);

// Examples

  // push works
  newUser.hobbies.push({
    name: 'child 2',
  });

  // hobbiesCopy type is any
  const hobbiesCopy = newUser.hobbies.toObject();

  // hobbies1: missing properties error
  const hobbies1: (typeof newUser)['hobbies'] = [
    {
      name: '',
    },
  ];

  // cannot use array methods
  newUser.hobbies.reverse();
  newUser.hobbies.filter((v) => v.name === 'test');
  newUser.hobbies.splice(0, 0, { name: 'prova' });

Address embedded

interface IAddress {
  street: string;
  city: string;
  zipcode: string;
}

type AddressDocumentOverrides = {};

interface IAddressVirtuals {}

type AddressInstance = HydratedSingleSubdocument<
  IAddress,
  IAddressVirtuals & AddressDocumentOverrides
>;

type AddressModelType = Model<
  IAddress,
  {},
  AddressDocumentOverrides,
  IAddressVirtuals,
  AddressInstance
>;

const addressSchema = new Schema<IAddress, AddressModelType>(
  {
    street: {
      type: SchemaTypes.String,
      required: true,
    },
    city: {
      type: SchemaTypes.String,
      required: true,
    },
    zipcode: {
      type: SchemaTypes.String,
      required: true,
    },
  },
  {}
);

// Examples

  // can push
  newUser.addresses.push({
    street: '123 Main St',
    city: 'New York',
    zipcode: '10001',
  });

  // addresses1: missing properties error
  const addresses1: (typeof newUser)['addresses'] = [
    {
      street: '456 Broadway',
      city: 'Los Angeles',
      zipcode: '90001',
    },
  ];

  // can use array methods but splice does not compile because of missing properties error
  newUser.addresses.reverse();
  newUser.addresses.filter((v) => v.street === 'test');
  newUser.addresses.splice(0, 0, {
    street: '120 Brooklyn',
    city: '',
    zipcode: '11220',
  });

Given these issues, is there a way to properly type embedded arrays? I still find it very challenging to work with and understand how to use them effectively.

@nikzanda nikzanda added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary help wanted labels Nov 15, 2024
@claudioaldrighettiatroos

I have the same problem. I confirm that the given solution to my issue can fix the error with .push(), but still doesn't fix the problem with methods like .filter(), .map() or .splice(). For methods like .map() the returned value is a normal js array and not a DocumentArray. Because of this, by trying to assign directly the returned array to the embedded field I still get a type error. It would be very useful and helpful to have a clear and complete solution to how embedded document arrays should be implemented. I would really appreciate it.

@vkarpov15
Copy link
Collaborator

First, the correct way to type your document arrays is the following. Use Types.DocumentArray<AddressInstance> not AddressInstance[].

type UserDocumentOverrides = {
  hobbies: Types.DocumentArray<HobbyInstance>;
  addresses: Types.DocumentArray<AddressInstance>;
};

I'm unable to repro your issue with newUser.hobbies.reverse(); not working, that compiles fine with the above changes. splice() doesn't work, but we added fix #15085 for that.

The following is not expected to work because the type of addresses is an array of hydrated document instances.

  // addresses1: missing properties error
  const addresses1: (typeof newUser)['addresses'] = [
    {
      street: '456 Broadway',
      city: 'Los Angeles',
      zipcode: '90001',
    },
  ];

@vkarpov15
Copy link
Collaborator

Re: map() returning a vanilla JS array, we will need to confirm the runtime behavior

@claudioaldrighettiatroos

First of all, thank you for checking the problem with splice().

I can understand the motivation behind the decision to make map() behave in this way: at the very end, its purpose is to transform every (or at least some) object of the array, thus it is right to do not expect an array of embedded document as return value.

The problem with reverse() is that it doesn't change anything but the order of its embedded documents, because of this it shouldn't change the nature of the array-wrapper-object that contains the embedded documents. Yes, it's true, the code above compiles with no errors associated to reverse(), but the returned value of that method is a vanilla JS array AddressInstance*[] (where AddressInstance* is Types.Subdocument<unknown, Record<string, never>, IAddress> & Omit<IAddress & { _id: Types.ObjectId; }, never> & IAddressVirtuals), not a Types.DocumentArray<AddressInstance*> object. Is it wrong to expect reverse() to return a Types.DocumentArray<T> instead of a vanilla Js array T[]?

vkarpov15 added a commit that referenced this issue Dec 11, 2024
types: add splice() to DocumentArray to allow adding partial objects with splice()
@vkarpov15 vkarpov15 reopened this Dec 11, 2024
@vkarpov15
Copy link
Collaborator

We will have to double check what happens at runtime. Mongoose doesn't explicitly implement map() or reverse() on arrays, so we need to check whether JavaScript returns a vanilla array in those cases or not.

It is worth mentioning that reverse() modifies the array in place, so you don't need to do arr = arr.reverse();, just arr.reverse(); is sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary
Projects
None yet
3 participants