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

feat: implement arrays of nested objects upload functionality #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxpeterson
Copy link

This PR adds the ability to upload an array of nested objects that can each have an (upload) file.

The use case is where you want to store a number of nested objects that can each have (upload) files. For example an array of photos on a (blog) post:

const photoSchema = new Schema({
  name: 'string',
  bucket: 'string',
  mime: 'string',
  key: 'string',
  size: 'number'
});
const postSchema = new Schema({
  name: 'string', 
  photos: [photoSchema],
});

To achieve this I have added a new option (parentArray) to the uploadFeature options. The parentArray option indicates that the (upload) feature should be nested under a parent array property.

For the above post example (with photos) the uploadFeature options would be something like:

  features: [uploadFeature({
    properties: { file: 'file', key: 'key', bucket: 'bucket', mimeType: 'mime', size: 'size' },
    parentArray: 'photos'
  })],

The code handles the parentArray case by building the properties "on the fly", to match the items array of photos. So file gets mapped to photos.0.file, photos.1.file, etc.,

I was hoping this would be achievable with fewer changes, but unfortunately it resulted in a fairly significant rewrite in particular of updateRecordFactory, which it now handles multiple uploads in a single call to updateRecord.

If you are interested in including this feature then I can add an example and tests for the new cases along with any improvements / alterations that you require, but I first wanted to get an indication of whether you see this as a worthwhile addition.

If not I could publish it as an upload-array-file feature.

Please let me know if you have some contribution or similar guidelines that I have missed.

Thanks for your time and for providing a great library!

@maxpeterson maxpeterson force-pushed the add-array-property-support branch from 681c869 to 10e7c8c Compare June 16, 2021 12:39
@jschroed91
Copy link

+1 on this feature -- just encountered use-case where this is necessary. I currently have a fork of your branch that we're using in our project for the time being, but would really like for that to only be a temporary solution until this feature is available in a release

@jschroed91
Copy link

@maxpeterson I have a use-case of having deeply nested objects where I'd like to handle file uploads at the 2nd level of nesting

E.g. a data structure like:

{
  "title": "A Book Title",
  "authors": [
    {
      "name": "Author A",
      "photos": [
        {
          "name": "...",
          "bucket": "...",
          "mime": "...",
          "key": "...",
          "size": 0
        },
        {
          "name": "...",
          "bucket": "...",
          "mime": "...",
          "key": "...",
          "size": 0
        }
      ]
    },
    {
      "name": "Author B",
      "photos": [
        {
          "name": "...",
          "bucket": "...",
          "mime": "...",
          "key": "...",
          "size": 0
        }
      ]
    }
  ]
}

Looking at the code I don't think these updates would support that use-case at the moment, does that sound correct?

Do you have any thoughts on how we could solve that use-case as well as part of this feature building off of what you have so far?

I know flat.get options (GetOptions) has an option available includeAllSiblings that could match a propertyPath like authors.photos and it would fetch all authors.N.photos elements.... so I was thinking maybe we could do something along those lines with the parentArray option if an optional flag is passed to includeAllSiblings? I'm not sure if we could easily get the full list of property keys from that though

@dziraf
Copy link
Contributor

dziraf commented Mar 28, 2022

@maxpeterson I'm alright with merging these changes if you resolve the conflicts. Thank you for contributing.

As far as multi-nesting is concerned, you can create a merge request which introduces this feature, but I'm worried that this library will be difficult to maintain if this gets very complex. Internally, we use it only for simple upload cases but use custom solutions for complex requirements

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.

3 participants