-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix conflicting File name and Promise resolve issue #19
Conversation
Reviewer's Guide by SourceryThis pull request addresses a bug in file handling and improves type safety. The main changes include refactoring the file serialization process to properly handle asynchronous operations, introducing separate types for client-side and server-side file objects, and moving type definitions to a dedicated file. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tengju - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more specific type for the
event
parameter inhandleFileInput
instead ofany
. This would improve type safety and make the function's expectations clearer.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -0,0 +1,13 @@ | |||
interface ServerFile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Align types between ServerFile and ClientFile
The types for 'size' and 'lastModified' in ServerFile are strings, while in ClientFile they are more specific (number). Consider aligning these types for consistency and to prevent potential type-related issues.
interface ServerFile {
name: string;
content: string;
size: number;
lastModified: number;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I mainly implemented this feature after the issue #13 and I wasn't very aware of this issue. I will approve it and push it into the next version as soon as I have time to test it
Thank you, if there is anything else please let me know. |
actually I was wondering if you have any experience with implementing Multipart Data Object upload since the current implementation uses Base64, I had a structure in mind to implement them both on file-storage.nuxt.space but I don't have enough time to properly implement it and test it since this is a breaking update that will be released on 0.3.0. If you don't have time for it I would like to get a reliable source to learn about it's implementations and best practices properly |
Hey, I'm not sure what you are asking. I like the JSON upload since I can now add more complex JSON schema and send them directly to Nuxt without setting up the whole logic myself. if you want to use the multipart data object just to upload files, then I don't think it is that important to read into it too much since nuxt already has their way of handling Multipart Data with the readMultipartFormData function. Try to think more about the problem you want to solve and give other developers the chance to make use of it. They will eventually come to ask for advice or suggest new features. Also, they will be able to present their use case in a more clear way in which you can learn how to better improve the DX of your module. |
Summary of the Bug:
The serializeFile function does not return a Promise, but in the handleFileInput function, you are treating it as if it does by pushing its result into the promises array and using Promise.all(promises) to wait for all the promises to resolve.
As a result, promises is an array of undefined values, and Promise.all(promises) resolves immediately, before the FileReader has completed reading the files. This means that handleFileInput resolves the promise before the files array is populated with the file contents, leading to unexpected behavior.
Solution:
Modify the serializeFile function to return a Promise that resolves when the file is read and processed. This way, handleFileInput can correctly wait for all files to be processed before resolving.
Refactor:
Since handleFileInput is already returning a promise and is designed to work with the promises from serializeFile, you can simplify the code by removing the unnecessary wrapping new Promise in handleFileInput.
removed the resolve and reject from handleFileInput since these are now being thrown in the serializeFile function
Generic improvements:
Summary by Sourcery
Fix the issue where serializeFile did not return a Promise, causing handleFileInput to resolve prematurely. Refactor the code to simplify promise handling and introduce type definitions for ServerFile and ClientFile to prevent naming conflicts.
Bug Fixes:
Enhancements:
Chores: