Skip to content

Add support for file versions#214

Draft
qgustavor wants to merge 2 commits intonextfrom
file-revision
Draft

Add support for file versions#214
qgustavor wants to merge 2 commits intonextfrom
file-revision

Conversation

@qgustavor
Copy link
Owner

@qgustavor qgustavor commented Sep 8, 2024

Calling .upload() on a file overrides it adding a new version.

Missing:

  • Handle naming: it should use the original file name I guess.
  • Handle how revisions work: at the moment the library is unaware of revisions and seems to always download the newest one.
  • Testing: testing on the real server, then implementing the server logic in mega-mock, then writing tests, then testing on real server again

@qgustavor
Copy link
Owner Author

I don't like this API: if we change .upload() to handle file revisions, it makes it messier. Some options I considered/consider:

  • Call .upload() on the parent folder, provide the overridden file as a parameter. It was the initial implementation, but seemed too not intuitive.
  • Call .upload() on the file, which is the current implementation, but you need to provide a file name on the first parameter anyway, which is not intuitive again.
  • Changing the current implementation to not require the first parameter in case of a file is being overridden can cause some TypeScript typing shenanigans I prefer to avoid.
  • Create an .uploadRevision() method instead? Like with .link() and .shareFolder(), one can just call the other method internally, but avoiding making the API non-intuitive.

@qgustavor qgustavor changed the title Add support for file revisions Add support for file versions Sep 8, 2024
@qgustavor
Copy link
Owner Author

qgustavor commented Sep 8, 2024

Implemented uploadVersion and changed the wording to match the original MEGA clients. The current implementation avoids the naming problem while being more intuitive. The current code is still unaware of file versions.

@adityaguru149
Copy link

Call .upload() on the parent folder, provide the overridden file as a parameter. It was the initial implementation, but seemed too not intuitive.

Why is this not preferred?
When in case of multiple clients trying to add similarly named files, it would be great to just overwrite the file with whoever does the upload later while also keeping a version of the other user.

@qgustavor
Copy link
Owner Author

@adityaguru149 .uploadVersion() implementation is just this:

  uploadVersion (source, originalCb) {
    if (this.directory) throw Error('node is a directory')
    return this.parent.upload({
      name: this.name,
      overridenNode: this
    }, source, originalCb)
  }

When working on this PR I guessed that file.uploadVersion(some_data) was better than folder.upload({ name: file.name, overridenNode: file }). The original idea still works, but I think uploadVersion is cleaner.

@adityaguru149
Copy link

@qgustavor

If I understood it correctly, basically this is for the case that we have established that there is already a version of the file in remote folder and we use it's name or handle. Makes sense.

My gripe with such approaches is that it would work well in most cases but might run issues when there are race conditions like say some client deleted the file while another is trying to add a version or when both think they are the first ones to upload the file.

Are features like list versions, read/download versions, cleanup versions of a file part of this PR or a separate PR?

@qgustavor
Copy link
Owner Author

@adityaguru149 I never finished researching this, you can finish it in a second PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants