Develop #129
Develop #129
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While your implementation is mostly solid and meets the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
File Existence Check: After checking if a file exists and sending a 404 response, the code should return immediately to prevent further execution. This is crucial to avoid any unintended behavior or errors.
-
File Handling Logic: There is a potential issue with the assignment of
filePathandfileName. Iffiles.fileis not an array, the code incorrectly assigns the entire file object. Ensure you access thefilepathandoriginalFilenameproperties directly in the else case to avoid this mistake. -
Content Encoding Headers: The content encoding headers are incorrect. For gzip, the header should be
'gzip', and for deflate, it should be'deflate'. Please update these headers to use the correct values.
Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you need any help. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
src/createServer.js
Outdated
| res.setHeader('Content-Encoding', 'gz'); | ||
| extension = 'gzip'; | ||
| break; | ||
| case 'deflate': | ||
| compressionData = createDeflate(); | ||
| res.setHeader('Content-Encoding', '.dfl'); |
There was a problem hiding this comment.
The content encoding headers are incorrect. For gzip, the correct header should be 'gzip' instead of 'gz'. Similarly, for deflate, the correct header should be 'deflate' instead of '.dfl'. Please update these headers to reflect the correct content encoding values.
There was a problem hiding this comment.
For gzip, I should use 'gz' because it doesn't pass the test.!!!! Similarly, for deflate, I should use '.dfl'
src/createServer.js
Outdated
| ? files.file[0].filepath | ||
| : files.file; | ||
| const fileName = Array.isArray(files.file) | ||
| ? files.file[0].originalFilename | ||
| : files.file; |
There was a problem hiding this comment.
There is a potential issue with the way filePath and fileName are assigned. If files.file is not an array, the code assigns the entire file object to filePath and fileName, which is likely incorrect. You should access the filepath and originalFilename properties directly in the else case.
There was a problem hiding this comment.
Thank you for your submission! Your code meets the task requirements and constraints, and you've done a great job implementing the server and HTML form. However, there are a few critical issues that need to be addressed to ensure the functionality of your code:
-
Content Encoding Headers:
- The content encoding header for Brotli should be
'br'instead of'.br'. - The content encoding header for deflate should be
'deflate'instead of'.dfl'. - The content encoding header for gzip should be
'gzip'instead of'gz'.
These are critical issues as they may cause clients to misinterpret the encoding.
- The content encoding header for Brotli should be
-
Validation of Uploaded Files and Compression Types:
- Ensure that
'files.file'is correctly parsed and validated. If'files.file'is not an array, this logic might fail. Consider adding additional validation to handle different data structures. - Similarly, ensure that
'fields.compressionType'is correctly parsed and validated. If'fields.compressionType'is not an array, this logic might fail.
- Ensure that
Despite these issues, the decision is to APPROVE your pull request to help you move forward with your learning. Please review the comments and make the necessary improvements to your code. Addressing these issues will enhance the robustness and correctness of your implementation. Keep up the good work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/createServer.js
Outdated
| switch (compressionType) { | ||
| case 'gzip': | ||
| compressionData = createGzip(); | ||
| res.setHeader('Content-Encoding', 'gz'); |
There was a problem hiding this comment.
The content encoding header for gzip should be 'gzip' instead of 'gz'. This is a critical issue as it may cause clients to misinterpret the encoding.
src/createServer.js
Outdated
| break; | ||
| case 'deflate': | ||
| compressionData = createDeflate(); | ||
| res.setHeader('Content-Encoding', '.dfl'); |
There was a problem hiding this comment.
The content encoding header for deflate should be 'deflate' instead of '.dfl'. This is a critical issue as it may cause clients to misinterpret the encoding.
src/createServer.js
Outdated
| break; | ||
| case 'br': | ||
| compressionData = createBrotliCompress(); | ||
| res.setHeader('Content-Encoding', '.br'); |
There was a problem hiding this comment.
The content encoding header for Brotli should be 'br' instead of '.br'. This is a critical issue as it may cause clients to misinterpret the encoding.
src/createServer.js
Outdated
| const compressionType = | ||
| Array.isArray(fields.compressionType) && fields.compressionType[0]; |
There was a problem hiding this comment.
Ensure that 'fields.compressionType' is correctly parsed and validated. If 'fields.compressionType' is not an array, this logic might fail. Consider adding additional validation to handle different data structures.
src/createServer.js
Outdated
| const compressionType = | ||
| Array.isArray(fields.compressionType) && fields.compressionType[0]; | ||
|
|
||
| const filePath = Array.isArray(files.file) && files.file[0].filepath; |
There was a problem hiding this comment.
Ensure that 'files.file' is correctly parsed and validated. If 'files.file' is not an array, this logic might fail. Consider adding additional validation to handle different data structures.
src/createServer.js
Outdated
| const fileName = | ||
| Array.isArray(files.file) && files.file[0].originalFilename; |
There was a problem hiding this comment.
Ensure that 'files.file' is correctly parsed and validated. If 'files.file' is not an array, this logic might fail. Consider adding additional validation to handle different data structures.
Hi, I sent scrins, can you check where was mistake with Content-Encoding? AI suggested me use gzip , but after this test won't pass...
