-
Notifications
You must be signed in to change notification settings - Fork 69
refactor: Improve Promise Handling, Validation, and Code Efficiency Across Scripts #1403
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
Changes from all commits
b3a4482
aa221b3
cabc7c4
4495e18
dd1adcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,40 +15,31 @@ exports.extractExtension = async function ( | |
| zipPath, | ||
| extensionsFolder = `${__dirname}/../extensions` | ||
| ) { | ||
| // Load in the archive with JSZip | ||
| const zip = await JSZip.loadAsync(await readFile(zipPath)).catch((e) => { | ||
| console.warn(`JSZip loading error caught: `, e); | ||
| return null; | ||
| }); | ||
| if (zip === null) 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' }; | ||
|
|
||
| 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 }; | ||
|
Comment on lines
18
to
+43
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The try catch block should only cover the block it is handling errors for (the |
||
| } | ||
|
|
||
| return { extensionName }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| /** @param {string} extensionName */ | ||
| exports.isValidExtensionName = (extensionName) => { | ||
| // Ensure extensionName is a string to prevent runtime errors | ||
| if (typeof extensionName !== 'string') return false; | ||
|
|
||
|
Comment on lines
+3
to
+5
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typechecking is already statically enforced via typescript: this is unnecessary complexity. |
||
| if (extensionName.length === 0) return false; | ||
|
|
||
| // Ensure that the first character is an uppercase character | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,19 @@ async function validate({ | |
| publicEventsFunctions, | ||
| onError, | ||
| }) { | ||
| // Check if extension name is a valid string | ||
| if (typeof name !== 'string') { | ||
| onError('Invalid extension name. Expected a string.'); | ||
| return; | ||
| } | ||
|
Comment on lines
+33
to
+36
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typechecking is already statically enforced via typescript: this is unnecessary complexity. |
||
|
|
||
| if (legacyCamelCaseExtensions.has(name)) return; | ||
| for (const { name } of publicEventsFunctions) checkPascalCase(name, onError); | ||
| for (const { name } of eventsBasedBehaviors) checkPascalCase(name, onError); | ||
|
|
||
| // Combine both loops into one to reduce duplication | ||
| const allFunctions = [...publicEventsFunctions, ...eventsBasedBehaviors]; | ||
| for (const { name } of allFunctions) { | ||
| checkPascalCase(name, onError); | ||
| } | ||
|
Comment on lines
+40
to
+44
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This offers no new benefit, does an inefficient copy, and unnecessarily complexifies the logic. |
||
| } | ||
|
|
||
| /** @type {import("./rule").RuleModule} */ | ||
|
|
||
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.
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.