refactor: migrate build script to typescript #3423
Conversation
WalkthroughThis pull request introduces extensive refactoring, migrating many scripts and configurations from JavaScript to TypeScript while transitioning to ES module syntax. Key changes include updates to ESLint rules and internationalization configuration, modifications in package.json scripts and dependencies, and improvements in error handling and logging across build, utility, and test files. New type definitions and declaration files are added, and numerous test imports and mocks are updated to align with TypeScript. Overall, the PR enhances type safety and standardizes module formats throughout the project. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant BM as buildMeetings Function
participant GA as Google API
participant FS as File System
U->>BM: Call buildMeetings(writePath)
BM->>GA: Authenticate & fetch calendar events
GA-->>BM: Return events list
BM->>BM: Process and validate events
BM->>FS: Write events to specified writePath
FS-->>BM: Confirm file write
BM-->>U: Return success or error response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
asyncapi-bot
requested review from
devilkiller-ag,
sambhavgupta0705,
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
…asyncapi-site into script-migration-2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/markdown/check-edit-links.ts (1)
186-187: Log errors usinglogger.errorbefore throwing in themainfunctionIn the
mainfunction, if an error occurs, you throw a new error without logging it. It's a good practice to log errors before throwing them to aid in debugging and monitoring.Apply this diff to improve error logging:
} catch (error) { - throw new Error(`Failed to check edit links: ${error}`); + logger.error(`Failed to check edit links: ${error}`); + throw new Error(`Failed to check edit links: ${error}`); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(5 hunks)scripts/markdown/check-edit-links.ts(5 hunks)tests/markdown/check-edit-links.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (3)
scripts/markdown/check-edit-links.ts (1)
2-2:⚠️ Potential issueCorrect the module name for 'node-fetch' import
On line 2, the module
'node-fetch-2'is being imported. The standard package name is'node-fetch'. Please verify the module name and ensure it matches the installed dependency in your project.Apply this diff to correct the import statement:
-import fetch from 'node-fetch-2'; +import fetch from 'node-fetch';If you intended to use a different package named
'node-fetch-2', please ensure it's properly installed and compatible with your codebase.tests/markdown/check-edit-links.test.js (2)
10-12: Ensure compatibility when requiring TypeScript files in JavaScript testsYou're requiring TypeScript files directly in your JavaScript test file. This can cause runtime errors if your testing environment doesn't support importing TypeScript modules directly. Consider one of the following:
- Compile the TypeScript files before running tests.
- Convert your test file to TypeScript for consistency.
- Adjust your test environment to handle TypeScript modules.
Please ensure that your test environment is configured to handle TypeScript modules appropriately.
Line range hint
2-2: Correct the module name for 'node-fetch' importOn line 2, the module
'node-fetch-2'is being required. The correct package name is'node-fetch'. Ensure that the package name matches the installed dependency to prevent runtime errors.Apply this diff to correct the require statement:
-const fetch = require('node-fetch-2'); +const fetch = require('node-fetch');If a different package named
'node-fetch-2'is intended, verify that it's installed and compatible with your test environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
scripts/markdown/check-edit-links.ts (7)
1-8: Consider using a more modern HTTP client.The
node-fetch-2package is outdated. Consider usingundici(Node.js built-in fetch) ornode-fetchv3 for better maintenance and security.-import fetch from 'node-fetch-2'; +import { fetch } from 'undici';
16-20: Enhance interface documentation.The
PathObjectinterface could benefit from JSDoc comments describing each property's purpose.+/** + * Represents a markdown file's paths and its corresponding edit link + */ interface PathObject { + /** The file system path to the markdown file */ filePath: string; + /** The URL path where the file will be served */ urlPath: string; + /** The GitHub edit link for the file, null if not applicable */ editLink: string | null; }
Line range hint
27-52: Improve error handling in processBatch.The error handling could be improved in several ways:
- The error is rejected without being logged
- The timeout logic could be extracted into a reusable utility
Consider this refactor:
+const checkWithTimeout = async (url: string, timeout: number) => { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), timeout); + try { + const response = await fetch(url, { + method: 'HEAD', + signal: controller.signal + }); + return response; + } finally { + clearTimeout(timeoutId); + } +}; async function processBatch(batch: PathObject[]): Promise<(PathObject | null)[]> { const TIMEOUT_MS = Number(process.env.DOCS_LINK_CHECK_TIMEOUT) || 5000; return Promise.all( batch.map(async ({ filePath, urlPath, editLink }) => { try { if (!editLink || ignoreFiles.some((ignorePath) => filePath.endsWith(ignorePath))) return null; - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), TIMEOUT_MS); - const response = await fetch(editLink, { - method: 'HEAD', - signal: controller.signal - }); - - clearTimeout(timeout); + const response = await checkWithTimeout(editLink, TIMEOUT_MS); if (response.status === 404) { return { filePath, urlPath, editLink }; } return null; } catch (error) { + logger.error(`Error checking ${editLink}:`, error); return Promise.reject(new Error(`Error checking ${editLink}: ${error}`)); } }) ); }
60-86: Improve type safety in checkUrls.The function could benefit from stronger typing and better error handling:
- The batch size should be validated
- The pause duration should be configurable
Consider this enhancement:
+const DEFAULT_BATCH_SIZE = 5; +const DEFAULT_PAUSE_DURATION = 1000; -async function checkUrls(paths: PathObject[]): Promise<PathObject[]> { +async function checkUrls( + paths: PathObject[], + options?: { + batchSize?: number; + pauseDuration?: number; + } +): Promise<PathObject[]> { const result: PathObject[] = []; - const batchSize = Number(process.env.DOCS_LINK_CHECK_BATCH_SIZE) || 5; + const batchSize = Math.max(1, Number(process.env.DOCS_LINK_CHECK_BATCH_SIZE) || options?.batchSize || DEFAULT_BATCH_SIZE); + const pauseDuration = Math.max(0, Number(process.env.DOCS_LINK_CHECK_PAUSE_DURATION) || options?.pauseDuration || DEFAULT_PAUSE_DURATION); // ... rest of the function ... - await pause(1000); + await pause(pauseDuration);
95-113: Improve type safety in determineEditLink.The function could benefit from a custom type for edit options and better null handling:
+interface EditOption { + /** The path segment to match against */ + value: string; + /** The base URL for edit links */ + href: string; +} function determineEditLink( urlPath: string, filePath: string, - editOptions: { value: string; href: string }[] + editOptions: EditOption[] ): string | null { // Remove leading 'docs/' if present for matching const pathForMatching = urlPath.startsWith('docs/') ? urlPath.slice(5) : urlPath; - const target = editOptions.find((edit) => pathForMatching.includes(edit.value)); + const target = editOptions.find((edit) => + edit.value === '' || pathForMatching.includes(edit.value) + ); - // Handle the empty value case (fallback) - if (target?.value === '') { + if (!target) { + return null; + } + + if (target.value === '') { return `${target.href}/docs/${urlPath}.md`; } - // For other cases with specific targets - /* istanbul ignore next */ - return target ? `${target.href}/${path.basename(filePath)}` : null; + return `${target.href}/${path.basename(filePath)}`; }
144-145: Remove unnecessary istanbul ignore.The
/* istanbul ignore else */comment seems unnecessary as theelsebranch is covered by the subsequentelse ifcondition.- /* istanbul ignore else */ - if (stats.isDirectory()) {
166-192: Improve main function configuration and error handling.The main function could benefit from:
- Configuration options for batch size and pause duration
- Better error logging before throwing
+interface CheckLinksOptions { + batchSize?: number; + pauseDuration?: number; + timeout?: number; +} -async function main() { +async function main(options?: CheckLinksOptions) { const editOptions = editUrls; try { const currentFilePath = fileURLToPath(import.meta.url); const currentDirPath = dirname(currentFilePath); const docsFolderPath = path.resolve(currentDirPath, '../../markdown/docs'); const paths = await generatePaths(docsFolderPath, editOptions); logger.info('Starting URL checks...'); - const invalidUrls = await checkUrls(paths); + const invalidUrls = await checkUrls(paths, { + batchSize: options?.batchSize, + pauseDuration: options?.pauseDuration + }); if (invalidUrls.length > 0) { logger.info('\nURLs returning 404:\n'); invalidUrls.forEach((url) => logger.info(`- ${url.editLink} generated from ${url.filePath}\n`)); logger.info(`\nTotal invalid URLs found: ${invalidUrls.length}`); } else { logger.info('All URLs are valid.'); } } catch (error) { + logger.error('Failed to check edit links:', error); throw new Error(`Failed to check edit links: ${error}`); } }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/validate-case-studies-structures.yaml (2)
4-4: Fix trailing spaces.Remove trailing spaces after
push:to maintain consistent code style.- push: + push:🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: trailing spaces
(trailing-spaces)
4-4: Consider adding path filters to the push trigger.The current configuration will run on all pushes, which could be inefficient. Consider adding path filters to only run when relevant files are changed:
push: paths: - 'config/casestudies/*.yml'🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 4-4: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/validate-case-studies-structures.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/validate-case-studies-structures.yaml
[error] 4-4: trailing spaces
(trailing-spaces)
…uest" This reverts commit cc194a6.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build-docs.ts (1)
115-117: Improve error handling for better type safety.The error handling could be more type-safe and informative.
- throw new Error(`Failed to build navigation tree: ${err}`); + throw new Error(`Failed to build navigation tree: ${err instanceof Error ? err.message : String(err)}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/build-docs.ts(1 hunks)
🔇 Additional comments (5)
scripts/build-docs.ts (5)
1-7: LGTM! Clean imports with good practices.The imports are well-organized, using named imports for better tree-shaking, and properly importing TypeScript types.
149-150: Fix error message construction and improve type safety.The error message construction is incorrect and the type assertion could be handled better.
244-245: Improve error message construction in addDocButtons.Similar to the previous error handling issue, the error message construction could be improved.
250-251: LGTM! Clean exports.The named exports are well-organized and follow TypeScript best practices.
201-215:⚠️ Potential issueAdd bounds checking for array access in navigation logic.
When constructing the next page title, ensure index + 2 is within bounds.
if (index + 1 < countDocPages) { if (!structuredPosts[index + 1].isRootElement && !structuredPosts[index + 1].isSection) { nextPage = { title: structuredPosts[index + 1].title, href: structuredPosts[index + 1].slug }; } else { + if (index + 2 < countDocPages) { nextPage = { title: `${structuredPosts[index + 1].title} - ${structuredPosts[index + 2].title}`, href: structuredPosts[index + 2].slug }; + } }Likely invalid or redundant comment.
| if (rootKey === 'reference' && key === 'specification') { | ||
| allChildren[key].item.href = childrenOfAllChildren.find((c) => c.isPrerelease === undefined)!.slug; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add fallback for specification version lookup.
The non-null assertion on the find result could be unsafe if no matching specification version is found.
🏁 Script executed:
#!/bin/bash
# Check if there are any specification versions without isPrerelease
rg -l "isPrerelease.*undefined" --type tsLength of output: 65
Potential Runtime Error in Specification Version Lookup
The code at lines 107–109 in scripts/build-docs.ts is still using a non-null assertion on the result of the find operation. If no matching specification version is found (i.e., if childrenOfAllChildren.find((c) => c.isPrerelease === undefined) returns undefined), accessing .slug will trigger a runtime error. Please add a conditional check or default fallback value to avoid this unsafe dereference.
- Check if the find result is defined before accessing
.slugor implement a fallback value.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
scripts/tools/combine-tools.ts (3)
67-112: 🛠️ Refactor suggestionExtract shared tag processing logic.
The language and technology processing logic is still duplicated. Consider extracting this into a shared function.
type TagType = 'language' | 'technology'; interface TagConfig { defaultColor: string; defaultBorderColor: string; fuseInstance: Fuse<LanguageColorItem>; tagList: LanguageColorItem[]; } async function processTag( tag: string, config: TagConfig ): Promise<LanguageColorItem> { const search = await config.fuseInstance.search(tag); if (search.length > 0) { return search[0].item; } const tagObject = { name: tag, color: config.defaultColor, borderColor: config.defaultBorderColor }; config.tagList.push(tagObject); config.fuseInstance = new Fuse(config.tagList, options); return tagObject; }Also applies to: 113-138
195-197:⚠️ Potential issueRemove non-null assertions from sort comparison.
The sort comparison still uses non-null assertions which could lead to runtime errors.
- finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => - tool!.title.localeCompare(anotherTool!.title) - ) as FinalAsyncAPITool[]; + finalTools[key].toolsList = [...automatedResults, ...manualResults].sort((tool, anotherTool) => { + if (!tool?.title || !anotherTool?.title) return 0; + return tool.title.localeCompare(anotherTool.title); + }) as FinalAsyncAPITool[];
200-204: 🛠️ Refactor suggestionEnhance error handling for file operations.
The file operations error handling could be more informative.
- fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); - fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList })); + try { + fs.writeFileSync(toolsPath, JSON.stringify(finalTools, null, 2)); + fs.writeFileSync(tagsPath, JSON.stringify({ + languages: languageList, + technologies: technologyList + }, null, 2)); + } catch (error) { + throw new Error( + `Failed to write output files:\n` + + `Tools path: ${toolsPath}\n` + + `Tags path: ${tagsPath}\n` + + `Error: ${error.message}` + ); + }scripts/tools/extract-tools-github.ts (1)
56-58:⚠️ Potential issueFix incorrect merging of items.
Line 56 incorrectly pushes all items into result.data.items, which already includes the first page of items, causing duplication.
- result.data.items.push(...allItems); + result.data.items = allItems;
🧹 Nitpick comments (4)
scripts/tools/combine-tools.ts (2)
20-23: Add error handling for schema validation setup.The schema validation setup could fail silently. Consider adding error handling:
-const ajv = new Ajv(); -addFormats(ajv, ['uri']); -const validate = ajv.compile(schema); +const ajv = new Ajv(); +try { + addFormats(ajv, ['uri']); + const validate = ajv.compile(schema); +} catch (error) { + throw new Error(`Failed to initialize schema validation: ${error.message}`); +}
35-40: Consider making Fuse configuration values configurable.The threshold value of 0.39 is hardcoded. Consider making it configurable for better flexibility:
+const FUSE_THRESHOLD = process.env.FUSE_THRESHOLD ? parseFloat(process.env.FUSE_THRESHOLD) : 0.39; + const options = { includeScore: true, shouldSort: true, - threshold: 0.39, + threshold: FUSE_THRESHOLD, keys: ['name', 'color', 'borderColor'] };scripts/tools/extract-tools-github.ts (2)
12-18: Add usage example to JSDoc.Consider adding an @example section to make the usage clearer for other developers.
/** * Fetches tool data from the GitHub API. * * @returns {Promise<ToolsData>} The data from the GitHub API. * @throws {Error} If there is an error fetching the data. + * @example + * const toolsData = await getData(); + * console.log(`Found ${toolsData.total_count} tools`); */
19-21: Enhance error message with setup instructions.The error message could be more helpful by including setup instructions.
- throw new Error('GITHUB_TOKEN environment variable is required'); + throw new Error( + 'GITHUB_TOKEN environment variable is required. Please set it in your .env file or environment variables. ' + + 'You can create a token at https://github.com/settings/tokens with `public_repo` scope.' + );🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 20-20: GITHUB_TOKEN environment variable is required
[error] 20-20: GITHUB_TOKEN environment variable is required
[error] 20-20: GITHUB_TOKEN environment variable is required
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/tools/combine-tools.ts(1 hunks)scripts/tools/extract-tools-github.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
scripts/tools/extract-tools-github.ts
[error] 60-60: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
🪛 GitHub Actions: PR testing - if Node project
scripts/tools/extract-tools-github.ts
[error] 20-20: GITHUB_TOKEN environment variable is required
[error] 20-20: GITHUB_TOKEN environment variable is required
[error] 20-20: GITHUB_TOKEN environment variable is required
🔇 Additional comments (4)
scripts/tools/combine-tools.ts (2)
68-68: Add optional chaining for filters access.This is still vulnerable to undefined filters.
-if (toolObject.filters.language) { +if (toolObject.filters?.language) {
142-166: LGTM! Well-structured error handling.The function has proper error handling, type safety, and clear logging of validation failures.
scripts/tools/extract-tools-github.ts (2)
1-10: LGTM! Imports and type declarations are well organized.The ESLint disable for no-await-in-loop is justified here since we need sequential pagination to avoid rate limits.
37-55: LGTM! Pagination handling is well implemented.Good practices implemented:
- Proper pagination based on total count
- Rate limiting protection with 1-second pause
- Progress logging
…asyncapi-site into script-migration-2
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/tools/extract-tools-github.ts (1)
12-17: Enhance JSDoc documentation.Consider adding
@requirestag to document the required environment variables./** * Fetches tool data from the GitHub API. * + * @requires {string} process.env.GITHUB_TOKEN - GitHub API token for authentication * @returns {Promise<ToolsData>} The data from the GitHub API. * @throws {Error} If there is an error fetching the data. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/tools/extract-tools-github.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
scripts/tools/extract-tools-github.ts
[error] 57-57: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
⏰ Context from checks skipped due to timeout of 180000ms (5)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Lighthouse CI
🔇 Additional comments (4)
scripts/tools/extract-tools-github.ts (4)
1-10: LGTM! Well-organized imports and configuration.The imports are properly organized and typed. The ESLint disable comment for
no-await-in-loopis justified as we need to wait between API calls to avoid rate limiting.
18-18: Add environment variable validation.Add validation for the required GITHUB_TOKEN environment variable at the start of the function.
export async function getData(): Promise<ToolsData> { + if (!process.env.GITHUB_TOKEN) { + throw new Error('GITHUB_TOKEN environment variable is required'); + }
19-58: Improve error handling.The current try-catch block only rethrows errors without adding value. Let's enhance it with proper error handling.
- // eslint-disable-next-line no-useless-catch - try { + try { const allItems = []; let page = 1; // ... rest of the code ... } catch (error) { - throw err; + if (axios.isAxiosError(error)) { + if (error.response?.status === 403) { + throw new Error('GitHub API rate limit exceeded. Please try again later.'); + } + if (error.response?.status === 401) { + throw new Error('Invalid GitHub token. Please check your GITHUB_TOKEN.'); + } + throw new Error(`GitHub API error: ${error.response?.data?.message || error.message}`); + } + throw error; }🧰 Tools
🪛 Biome (1.9.4)
[error] 57-57: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.(lint/complexity/noUselessCatch)
53-53: Fix incorrect merging of items.There's a bug in how items are merged. The line
result.data.items.push(...allItems)is incorrect asallItemsalready contains all items including those from the first page.-result.data.items.push(...allItems); +result.data.items = allItems;
Description
✅ Deploy Preview for asyncapi-website ready!
Built without sensitive environment variables
Toggle QR Code...
Use your smartphone camera to open QR code link.
To edit notification comments on pull requests, go to your Netlify site configuration.
Related issue(s)
Related to #3187
Summary by CodeRabbit
New Features
Refactor & Chores