refactor: Improve Promise Handling, Validation, and Code Efficiency Across Scripts #1403
Conversation
|
hey @4ian , why is this check not passing.. i tried fixing the issue using npx prettier --write "scripts//*.js" "scripts//*.ts" and it's still not working.. |
| // Combine both loops into one to reduce duplication | ||
| const allFunctions = [...publicEventsFunctions, ...eventsBasedBehaviors]; | ||
| for (const { name } of allFunctions) { | ||
| checkPascalCase(name, onError); | ||
| } |
There was a problem hiding this comment.
This offers no new benefit, does an inefficient copy, and unnecessarily complexifies the logic.
| if (typeof name !== 'string') { | ||
| onError('Invalid extension name. Expected a string.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Typechecking is already statically enforced via typescript: this is unnecessary complexity.
| // Ensure extensionName is a string to prevent runtime errors | ||
| if (typeof extensionName !== 'string') return false; | ||
|
|
There was a problem hiding this comment.
Typechecking is already statically enforced via typescript: this is unnecessary complexity.
| try { | ||
| // Load in the archive with JSZip | ||
| const zip = await JSZip.loadAsync(await readFile(zipPath)); | ||
| if (!zip) return { error: 'zip-error' }; | ||
| // Find JSON files in the zip top level folder | ||
| const jsonFiles = zip.file(/.*\.json$/); | ||
| // Ensure there is exactly 1 file | ||
| if (jsonFiles.length === 0) return { error: 'no-json-found' }; | ||
| if (jsonFiles.length > 1) return { error: 'too-many-files' }; | ||
| const [file] = jsonFiles; | ||
| const { name: extensionName, dir, ext } = parsePath(file.name); | ||
| if (ext !== '.json') return { error: 'invalid-file-name' }; | ||
| // Ensure no special characters are in the extension name to prevent relative path | ||
| // name shenanigans with dots that could put the extension in the reviewed folder. | ||
| if (dir) return { error: 'invalid-file-name' }; | ||
| if (!isValidExtensionName(extensionName)) | ||
| return { error: 'invalid-file-name' }; | ||
| // Write the extension to the community extensions folder | ||
| await pipeline( | ||
| file.nodeStream(), | ||
| createWriteStream(`${extensionsFolder}/community/${file.name}`) | ||
| ); | ||
| return { extensionName }; | ||
| } catch (e) { | ||
| console.warn(`JSZip extraction error caught: `, e); | ||
| return { error: 'zip-error' }; | ||
| console.warn(`JSZip loading or extraction error caught: `, e); | ||
| return { error: 'zip-error', details: e.message }; |
There was a problem hiding this comment.
The try catch block should only cover the block it is handling errors for (the JSZip.loadAsync call)
| } catch (error) { | ||
| // General error catch for any unhandled async errors in the process | ||
| console.error(`Error verifying extension: ${extensionName}`, error); | ||
| return { code: 'not-found' }; // Return a default error code, you could also return something more specific based on the context |
There was a problem hiding this comment.
Most definitely not! The error code determines what error message is sent back to the submitter - sending them a message about a file not being found is not generic, and goes against the point of providing feedback. The script crashing is already a handled case, so this unnecessarily strips the context of the error.
|
Since every proposed change here does not in fact improve promise handling, validation or code efficiency, I'll close this PR. |
2 participants
Pull Request Description :
Closes #1402
This pull request refactors and improves several scripts in the project by enhancing error handling, improving validation logic, and streamlining promise management. Below are the key changes made across different files:
verifyExtension.js:try-catchblock for consistent async error handling across the function..catch()andasync/awaitwith a more streamlined approach, ensuring any unhandled async errors are caught and logged properly.extract-extension.js:try-catchblock.ExtensionNameValidator.js:extensionNameto ensure it is a string before proceeding with further validation.PascalCase.js:publicEventsFunctionsandeventsBasedBehaviors. This reduces code duplication and simplifies the logic.extension.nameto ensure it is a string before validating the PascalCase, preventing errors when the name is not provided or is of the wrong type.Benefits: