fix: update code to support @octokit/request v9 and @octokit/graphql v8#3697
fix: update code to support @octokit/request v9 and @octokit/graphql v8#3697AritraDey-Dev wants to merge 30 commits intoasyncapi:masterfrom
Conversation
AritraDey-Dev
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve and
derberg
as code owners
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Lambda Handler
participant Octokit as @octokit/graphql
participant GitHub as GitHub API
Handler->>Octokit: graphql(query, { auth: GITHUB_TOKEN })
Octokit->>GitHub: Execute GraphQL query with token
GitHub-->>Octokit: Return data
Octokit-->>Handler: Return data
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
AritraDey-Dev
changed the title
|
@AritraDey-Dev Make sure all the tests and build pass |
AritraDey-Dev
force-pushed
the
fix-octokit-upgrade
branch
from
1320dd5 to
2cad54c
Compare
This reverts commit 2cad54c.
![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
🔭 Outside diff range comments (1)
scripts/dashboard/build-dashboard.js (1)
180-180:⚠️ Potential issueUpdate the module execution check for ES modules
The
require.main === modulecheck is specific to CommonJS and won't work in an ES module environment. This line needs to be updated to the ES module equivalent.- if (require.main === module) { + if (import.meta.url === `file://${process.argv[1]}`) {
🧹 Nitpick comments (1)
scripts/dashboard/build-dashboard.js (1)
184-184: Successfully migrated to ES module exportsThe export statement has been correctly migrated from CommonJS
module.exportsto ES moduleexport.For better readability, consider formatting the exports with each name on a separate line as suggested by the linter:
- export { getLabel, monthsSince, mapGoodFirstIssues, getHotDiscussions, getDiscussionByID, getDiscussions, writeToFile, start, processHotDiscussions, pause }; + export { + getLabel, + monthsSince, + mapGoodFirstIssues, + getHotDiscussions, + getDiscussionByID, + getDiscussions, + writeToFile, + start, + processHotDiscussions, + pause + };🧰 Tools
🪛 ESLint
[error] 184-184: Replace
·getLabel,·monthsSince,·mapGoodFirstIssues,·getHotDiscussions,·getDiscussionByID,·getDiscussions,·writeToFile,·start,·processHotDiscussions,·pause·with⏎··getLabel,⏎··monthsSince,⏎··mapGoodFirstIssues,⏎··getHotDiscussions,⏎··getDiscussionByID,⏎··getDiscussions,⏎··writeToFile,⏎··start,⏎··processHotDiscussions,⏎··pause⏎(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/dashboard/build-dashboard.js(2 hunks)
🧰 Additional context used
🪛 ESLint
scripts/dashboard/build-dashboard.js
[error] 4-4: Unexpected use of file extension "js" for "./issue-queries.js"
(import/extensions)
[error] 184-184: Replace ·getLabel,·monthsSince,·mapGoodFirstIssues,·getHotDiscussions,·getDiscussionByID,·getDiscussions,·writeToFile,·start,·processHotDiscussions,·pause· with ⏎··getLabel,⏎··monthsSince,⏎··mapGoodFirstIssues,⏎··getHotDiscussions,⏎··getDiscussionByID,⏎··getDiscussions,⏎··writeToFile,⏎··start,⏎··processHotDiscussions,⏎··pause⏎
(prettier/prettier)
🔇 Additional comments (1)
scripts/dashboard/build-dashboard.js (1)
1-4: Successfully migrated to ES module importsThe imports have been properly migrated from CommonJS
requirestatements to ES moduleimportstatements, which aligns with the PR objective to update Octokit dependencies.Note: The ESLint warning about file extensions is a configuration issue. Including
.jsextensions in imports is actually required for ES modules in Node.js environments.🧰 Tools
🪛 ESLint
[error] 4-4: Unexpected use of file extension "js" for "./issue-queries.js"
(import/extensions)
|
@AritraDey-Dev The testing workflow is failing. Are you still willing to continue on this? |
Apologies for the delay. I will look into a possible fix. Do you have any insights on why the tests are failing? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
netlify/functions/github_discussions.ts(1 hunks)netlify/functions/save-discussion-background/helpers.ts(1 hunks)package.json(1 hunks)tests/dashboard/build-dashboard.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code Definitions (1)
tests/dashboard/build-dashboard.test.js (5)
tests/casestudies/index.test.js (2) (2)
tempDir(7-7)fs(1-1)tests/markdown/check-markdown.test.js (2) (2)
tempDir(18-18)fs(1-1)tests/build-post-list.test.js (2) (2)
tempDir(7-7)fs(1-1)tests/build-tools.test.js (1) (1)
fs(3-3)tests/readAndWriteJson.test.js (1) (1)
fs(1-1)
🪛 ESLint
tests/dashboard/build-dashboard.test.js
[error] 12-12: 'processHotDiscussions' is defined but never used.
(no-unused-vars)
[error] 13-13: 'pause' is defined but never used.
(no-unused-vars)
[error] 14-14: Unexpected use of file extension "js" for "./build-dashboard.js"
(import/extensions)
🔇 Additional comments (4)
netlify/functions/save-discussion-background/helpers.ts (1)
21-21: Authorization header updated to use Bearer tokenThe authorization scheme has been correctly updated from
tokentoBearer, which aligns with the OAuth 2.0 standard and is required for compatibility with the newer version of@octokit/graphql(v8.2.1).This change ensures proper authentication with GitHub's GraphQL API v4, which prefers the Bearer token format.
tests/dashboard/build-dashboard.test.js (3)
37-38: File system operations updated to use promises APIThe temporary directory creation has been properly updated to use the asynchronous
fs.mkdirwith therecursiveoption, which is more robust than the previous synchronous version.This change aligns with modern JavaScript practices for handling file system operations using promises instead of callbacks or synchronous methods.
42-42: File system cleanup updated to use promises APIThe temporary directory removal has been correctly updated to use the asynchronous
fs.rmmethod with therecursiveandforceoptions, which is better than the previous synchronous version.This change ensures proper cleanup after tests using modern promise-based file operations.
84-90: Test expectation updated for the rate limit warningThe test expectation for rate limit warnings has been updated to check for multiple arguments in the
console.logcall instead of a single string. This update is likely required due to changes in how logging happens in the updated version of the library.The test now properly verifies each component of the rate limit warning message, which improves test specificity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/dashboard/build-dashboard.test.js (1)
12-12: Add file extension to import pathAccording to ESLint rules and ES module best practices, the file extension should be included in the import path.
- } from './build-dashboard';// Adjust the import path as necessary + } from './build-dashboard.js';// Adjust the import path as necessaryAdditionally, add a space before the comment for proper formatting:
- } from './build-dashboard';// Adjust the import path as necessary + } from './build-dashboard.js'; // Adjust the import path as necessary🧰 Tools
🪛 ESLint
[error] 12-12: Missing file extension for "./build-dashboard"
(import/extensions)
[error] 12-12: Insert
·(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netlify/functions/github_discussions.ts(1 hunks)tests/dashboard/build-dashboard.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netlify/functions/github_discussions.ts
🧰 Additional context used
🪛 ESLint
tests/dashboard/build-dashboard.test.js
[error] 12-12: Missing file extension for "./build-dashboard"
(import/extensions)
[error] 12-12: Insert ·
(prettier/prettier)
🔇 Additional comments (5)
tests/dashboard/build-dashboard.test.js (5)
1-3: LGTM: Proper ES module import syntaxThe changes correctly update the import statements to use ES module syntax, which aligns with the PR's objective of updating Octokit dependencies to newer versions (v8 and v9) that likely require ES modules.
35-37: LGTM: Using Promise-based file system APIGood update to use the Promise-based
fs.mkdirwith therecursive: trueoption, which is more appropriate for async Jest test environments and aligns with modern JavaScript practices.
40-40: LGTM: Using Promise-based file system API for cleanupGood update to use the Promise-based
fs.rmwith appropriate options for thorough cleanup after tests.
82-88: LGTM: Updated assertion for console.log formatThe updated assertion correctly matches the new format of rate limit warning logs, which now uses multiple arguments instead of a single formatted string. This makes the test more specific and robust.
179-187:Details
✅ Verification successful
Verify if processHotDiscussions import is needed
This test is checking functionality for
processHotDiscussions, but this function isn't included in the imports at the top of the file. Previous review comments indicated it was unused, but this test suggests it might be needed.
🏁 Script executed:
#!/bin/bash # Check if processHotDiscussions is used in build-dashboard.js and if it's exported # First, check if the function exists in build-dashboard.js echo "Checking if processHotDiscussions is defined in build-dashboard.js:" rg -A 2 -B 2 "function\s+processHotDiscussions" ./scripts/dashboard/build-dashboard.js || rg -A 2 -B 2 "const\s+processHotDiscussions" ./scripts/dashboard/build-dashboard.js # Then check if it's exported echo "Checking if processHotDiscussions is exported:" rg -A 2 -B 2 "export\s+.*processHotDiscussions" ./scripts/dashboard/build-dashboard.jsLength of output: 695
ProcessHotDiscussions is correctly integrated via getHotDiscussions
- Verification confirms that
processHotDiscussionsis defined and exported inscripts/dashboard/build-dashboard.js.- The test in
tests/dashboard/build-dashboard.test.jsexercises the error handling forprocessHotDiscussionsindirectly throughgetHotDiscussions, so a direct import isn’t required.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3697 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you for reviewing this, @akshatnema. |
| transformIgnorePatterns: [ | ||
| "/node_modules/(?!(@octokit|universal-user-agent|before-after-hook|@github|is-plain-object|@sindresorhus|deprecation)/)" | ||
| ] |
There was a problem hiding this comment.
Why has this pattern been ignored in the jest config?
There was a problem hiding this comment.
The pattern is used to ensure Jest transforms specific modern ES modules from node_modules (like @octokit) that would otherwise cause syntax errors due to untranspiled import/export statements.
There was a problem hiding this comment.
The issue with the failing tests was due solely to the octokit module. After addressing it, the tests are now passing as expected.
| moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json'] | ||
| moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json'], | ||
| transformIgnorePatterns: [ | ||
| '/node_modules/(?!((@[^/]+/[^/]+)|universal-user-agent|is-plain-object)/)', |
There was a problem hiding this comment.
@akshatnema I’ve updated the pattern to be more generalized for handling ESM packages in node_modules.
Would love to hear your feedback on this approach.
|
@AritraDey-Dev please update the project |
| "typescript": "^5.3.3" | ||
| } | ||
| } | ||
| } No newline at end of file |
|
This pull request has been automatically marked as stale because it has not had recent activity 😴 It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation. There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model. Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here. Thank you for your patience ❤️ |
|
Thanks for the PR @AritraDey-Dev. Please re-open the PR with comments resolved if you are still working on the project. |
github-project-automation
bot
moved this from To Be Triaged
to Done
in Website - Kanban
|
@anshgoyalevil I have fixed it in , #5171 Please have a look. |
6 participants
fixes #3690
Description
Update
package.jsonto support@octokit/requestv9.2.2 and@octokit/graphqlv8.2.1.@octokit/graphqlto version8.2.1.@octokit/requestwith version9.2.2.Summary by CodeRabbit