Upgrade inquirer version to 11.0.2 #2720
Conversation
WalkthroughThis pull request focuses on upgrading the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/setup/askForCustomPort/askForCustomPort.test.ts (1)
13-13: LGTM on type changes, but test coverage needs improvement.The type changes from string to number expectations are correct, but the test suite would benefit from additional test cases.
Consider adding the following test cases:
test('should reject invalid port number', async () => { jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ customPort: 'invalid' }); await expect(askForCustomPort()).rejects.toThrow(); }); test('should handle boundary values', async () => { // Test minimum port jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ customPort: '1' }); await expect(askForCustomPort()).resolves.toBe(1); // Test maximum port jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ customPort: '65535' }); await expect(askForCustomPort()).resolves.toBe(65535); });Also applies to: 22-22
src/setup/askForCustomPort/askForCustomPort.ts (1)
20-20: Consider error handling for edge cases.The number conversion is done twice - once in validation and once in return.
Consider refactoring to:
- return Number(customPort); + const port = Number(customPort); + if (Number.isNaN(port)) { + throw new Error('Invalid port number provided'); + } + return port;src/setup/askForTalawaApiUrl/askForTalawaApiUrl.test.ts (1)
Line range hint
8-49: Consider adding error case testsThe current tests only cover successful scenarios. Consider adding test cases for:
- Invalid URL formats
- Network-related error handling
- User cancellation scenarios
Example test case:
test('should handle invalid URL format', async () => { const mockPrompt = jest.spyOn(inquirer, 'prompt').mockResolvedValueOnce({ endpoint: 'invalid-url', }); await expect(askForTalawaApiUrl()).rejects.toThrow('Invalid URL format'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/setup/askForCustomPort/askForCustomPort.test.ts(2 hunks)src/setup/askForCustomPort/askForCustomPort.ts(1 hunks)src/setup/askForTalawaApiUrl/askForTalawaApiUrl.test.ts(1 hunks)src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/setup/askForCustomPort/askForCustomPort.ts
[error] 13-13: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (6)
src/setup/askForCustomPort/askForCustomPort.ts (2)
4-4: LGTM on type safety improvements.
Good use of TypeScript generics with inquirer and explicit string type for customPort.
Also applies to: 10-10
1-21: Verify inquirer upgrade compatibility.
Since this is part of an inquirer upgrade from 8.0.0 to 11.0.2, we should verify the prompt behavior hasn't changed.
✅ Verification successful
Inquirer upgrade appears safe for current usage patterns
The codebase uses inquirer consistently across all files with standard prompt patterns (input, confirm) and basic validation. The upgrade from 8.0.0 to 11.0.2 doesn't affect these core features as:
- All prompt usages follow the same structure with standard properties (type, name, message, validate)
- Input validation patterns are simple and use basic JavaScript, not inquirer-specific features
- No usage of advanced or deprecated inquirer features that might have changed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in inquirer v11 that might affect prompts
rg -l "inquirer.*prompt" | xargs rg "type.*input.*validate"
# Check package.json for version
cat package.json | jq '.dependencies.inquirer, .devDependencies.inquirer'
Length of output: 152
Script:
#!/bin/bash
# Search for all inquirer prompt usages to verify consistent patterns
rg "inquirer\.prompt.*validate" -A 5 -B 5
# Check git history for any recent changes or fixes related to inquirer
git log --oneline -n 10 | grep -i "inquirer"
Length of output: 43
Script:
#!/bin/bash
# Look for all inquirer prompt usages with broader pattern
rg "inquirer\.prompt" -A 10 -B 2
# Find all files that import inquirer
rg "import.*inquirer" --type ts
Length of output: 5577
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts (2)
4-4: Type safety improvement looks good!
The addition of the type parameter <{ endpoint: string }> to inquirer.prompt improves type safety by ensuring the response structure matches expectations.
Line range hint 4-11: Verify compatibility with inquirer v11
The upgrade from inquirer v8 to v11 includes breaking changes. Please verify that:
- The prompt options structure remains compatible
- The return type structure hasn't changed
✅ Verification successful
Let me gather more information about the inquirer package version and its usage.
Based on the gathered information, I can now provide the final response:
The code is compatible with inquirer v11
The current implementation using inquirer.prompt<{ endpoint: string }>([...]) with the options structure containing type, name, message, and default properties is fully compatible with inquirer v11. The package.json confirms the project is already using inquirer v11.0.2, and all usages across the codebase follow the same compatible pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all inquirer.prompt usages to ensure consistent updates
rg "inquirer\.prompt" --type ts
Length of output: 716
Script:
#!/bin/bash
# Check package.json for inquirer version
cat package.json | jq '.dependencies.inquirer, .devDependencies.inquirer'
# Get the content of files using inquirer to check the pattern
cat src/setup/askForTalawaApiUrl/askForTalawaApiUrl.ts
cat src/setup/askForCustomPort/askForCustomPort.ts
cat setup.ts
Length of output: 7190
src/setup/askForTalawaApiUrl/askForTalawaApiUrl.test.ts (1)
33-35: Mock implementation improvement looks good!
The change to use mockResolvedValueOnce simplifies the test and better represents the async nature of the prompt.
package.json (1)
41-41: Verify inquirer upgrade compatibility
The major version upgrade from 8.x to 11.x may have breaking changes. Please verify:
- Peer dependency compatibility
- Type definitions compatibility with @types/inquirer
✅ Verification successful
Inquirer upgrade is compatible with current usage
The codebase usage of inquirer is compatible with v11:
- All usage patterns (
prompt()withtype,name, and simple question objects) are supported in v11 - Types are correctly aligned (@types/inquirer v9 matches inquirer v11)
- Usage is simple and doesn't involve deprecated features
- Node.js engine requirement (>=18) is satisfied as per package-lock.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for peer dependency warnings
npm ls inquirer
# Check type compatibility
tsc --noEmit src/setup/**/*.ts
Length of output: 725
Script:
#!/bin/bash
# Search for inquirer usage patterns in the codebase
rg "inquirer\." -A 2
# Find files importing inquirer
rg "import.*inquirer"
# Check package-lock.json for actual resolved version
cat package-lock.json | jq '.packages["node_modules/inquirer"]'
# Check @types/inquirer version
cat package.json | jq '.devDependencies["@types/inquirer"]'
Length of output: 2608
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2720 +/- ##
====================================================
+ Coverage 79.58% 86.38% +6.79%
====================================================
Files 295 312 +17
Lines 7274 8152 +878
Branches 1592 1791 +199
====================================================
+ Hits 5789 7042 +1253
+ Misses 1222 933 -289
+ Partials 263 177 -86 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/setup/askForCustomPort/askForCustomPort.ts (2)
3-9: Consider additional port validation checksThe validation logic is good, but could be enhanced with additional checks:
- Warning for privileged ports (0-1023)
- Warning for commonly used ports (e.g., 3000, 8080)
Consider adding these checks:
export function validatePort(input: string): string | true { const port = Number(input); if (Number.isNaN(port) || port <= 0 || port > 65535) { return 'Please enter a valid port number between 1 and 65535.'; } + if (port <= 1023) { + return 'Warning: Ports below 1024 are privileged and may require elevated permissions.'; + } + const commonPorts = [3000, 8080, 4200, 5000]; + if (commonPorts.includes(port)) { + return 'Warning: This is a commonly used port. It might conflict with other services.'; + } return true; }
14-30: Add escape mechanism to the input loopThe while loop could potentially run indefinitely if the user keeps entering invalid input. Consider adding a way to exit the prompt after a certain number of attempts or via a specific input.
Consider implementing a retry limit:
export async function askForCustomPort(): Promise<number> { let validPort: number | null = null; + let attempts = 0; + const MAX_ATTEMPTS = 3; - while (validPort === null) { + while (validPort === null && attempts < MAX_ATTEMPTS) { + attempts++; const { customPort } = await inquirer.prompt<{ customPort: string }>([ { type: 'input', name: 'customPort', message: 'Enter custom port for development server (leave blank for default 4321):', default: '4321', validate: validatePort, }, ]); validPort = Number(customPort); } + + if (validPort === null) { + console.log('Maximum attempts reached. Using default port 4321.'); + return 4321; + } return validPort; }src/setup/askForCustomPort/askForCustomPort.test.ts (1)
25-35: Enhance validation test coverageWhile the current validation tests cover basic scenarios, consider adding more edge cases:
Add these test cases:
test('should validate edge case port numbers', () => { // Edge cases expect(validatePort('1')).toBe(true); expect(validatePort('65535')).toBe(true); expect(validatePort('65536')).toBe('Please enter a valid port number between 1 and 65535.'); // Decimal numbers expect(validatePort('3000.5')).toBe('Please enter a valid port number between 1 and 65535.'); // Empty input expect(validatePort('')).toBe('Please enter a valid port number between 1 and 65535.'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/setup/askForCustomPort/askForCustomPort.test.ts(3 hunks)src/setup/askForCustomPort/askForCustomPort.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/setup/askForCustomPort/askForCustomPort.ts (2)
3-9: LGTM! Consider adding type assertion for better type safety.The validation logic is sound and follows best practices. For better type safety, consider adding a type assertion:
-export function validatePort(input: string): string | true { +export function validatePort(input: string): string | boolean { const port = Number(input); - if (Number.isNaN(port) || port <= 0 || port > 65535) { + if (Number.isNaN(port) || !Number.isInteger(port) || port <= 0 || port > 65535) { return 'Please enter a valid port number between 1 and 65535.'; } return true as const; }
24-53: Consider extracting magic numbers into named constants.For better maintainability and consistency, consider extracting the hardcoded values:
+const DEFAULT_PORT = 4321; +const MAX_RETRY_ATTEMPTS = 5; +const RESERVED_PORT_THRESHOLD = 1024; export async function askForCustomPort(): Promise<number> { - let MAX_ATTEMPTS = 5; + let remainingAttempts = MAX_RETRY_ATTEMPTS; - while (MAX_ATTEMPTS--) { + while (remainingAttempts--) { // ... rest of the code ... - default: '4321', + default: DEFAULT_PORT.toString(), // ... rest of the code ... } - return 4321; + return DEFAULT_PORT; }src/setup/askForCustomPort/askForCustomPort.test.ts (1)
25-98: Consider organizing tests into logical groups using describe blocks.The tests are comprehensive but could be better organized for clarity:
describe('askForCustomPort', () => { + describe('basic port validation', () => { test('should return default port if user provides no input', ...); test('should return user-provided port', ...); test('should return validation error if port not between 1 and 65535', ...); + }); + describe('retry mechanism', () => { test('should handle invalid port input and prompt again', ...); test('should return default port after maximum retry attempts', ...); + }); + describe('reserved ports', () => { test('should return user-provided port after confirming reserved port', ...); test('should re-prompt user for port if reserved port confirmation is denied', ...); test('should return default port if reserved port confirmation is denied after maximum retry attempts', ...); + }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/setup/askForCustomPort/askForCustomPort.test.ts(3 hunks)src/setup/askForCustomPort/askForCustomPort.ts(1 hunks)
🔇 Additional comments (2)
src/setup/askForCustomPort/askForCustomPort.ts (2)
11-22: LGTM! Well-structured warning prompt for reserved ports.
The function properly handles user confirmation for reserved ports with a secure default of false.
24-53: Verify the default port usage across the codebase.
Let's ensure the default port value is consistently used across the codebase.
✅ Verification successful
Let me check the content of the configuration files to ensure port consistency.
Default port 4321 is consistently used across the codebase
The verification shows that the default port 4321 is consistently used in:
- Development server configuration (
config/vite.config.ts) - Port selection logic (
src/setup/askForCustomPort/askForCustomPort.ts) - Related test files
The only other occurrence (0987654321) is unrelated as it's a phone number in a test file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded port numbers
rg -n "4321|'4321'|\"4321\"" --type ts
# Search for port-related configuration
fd -e json -e yaml -e yml -e config -x grep -l "port"
Length of output: 1321
Script:
#!/bin/bash
# Check content of configuration files for port-related settings
cat config/vite.config.ts
cat biome.json
Length of output: 2589
Aad1tya27
force-pushed
the
upgrade-inquirer-version
branch
from
b798df4 to
9be9ea4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/setup/askForCustomPort/askForCustomPort.ts (2)
6-17: Consider adding input type validationThe port validation is thorough but could be enhanced with input type checking.
Consider this enhancement:
export function validatePort(input: string): string | boolean { + if (typeof input !== 'string') { + return 'Input must be a string'; + } const port = Number(input); if ( Number.isNaN(port) ||
32-62: Consider enhancing error handling and loggingWhile the implementation is solid, there are opportunities for improvement:
- Add error logging for failed attempts
- Consider adding a debug mode for troubleshooting
Here's a suggested enhancement:
export async function askForCustomPort(): Promise<number> { let remainingAttempts = MAX_RETRY_ATTEMPTS; + const debug = process.env.DEBUG === 'true'; while (remainingAttempts--) { + if (debug) { + console.log(`Attempt ${MAX_RETRY_ATTEMPTS - remainingAttempts} of ${MAX_RETRY_ATTEMPTS}`); + } try { const { customPort } = await inquirer.prompt<{ customPort: string }>([ { type: 'input', name: 'customPort', message: `Enter custom port for development server (leave blank for default ${DEFAULT_PORT}):`, default: DEFAULT_PORT.toString(), validate: validatePort, }, ]); if (customPort && validatePort(customPort) === true) { if (Number(customPort) >= 1024) { return Number(customPort); } if ( Number(customPort) < 1024 && (await reservedPortWarning(Number(customPort))) ) { return Number(customPort); } } + } catch (error) { + console.error('Error during port selection:', error); + } } console.log( `\nMaximum attempts reached. Using default port ${DEFAULT_PORT}.`, ); return DEFAULT_PORT; }src/setup/askForCustomPort/askForCustomPort.test.ts (2)
11-41: Consider adding edge case testsWhile the basic validation tests are good, consider adding these edge cases:
test('should handle floating point numbers', () => { expect(validatePort('8080.5')).toBe( 'Please enter a valid port number between 1 and 65535.', ); }); test('should handle empty string', () => { expect(validatePort('')).toBe( 'Please enter a valid port number between 1 and 65535.', ); });
69-108: Consider adding boundary tests for reserved portsThe reserved ports tests are thorough, but consider adding boundary tests:
test('should handle port 1023 as reserved', async () => { jest .spyOn(inquirer, 'prompt') .mockResolvedValueOnce({ customPort: '1023' }) .mockResolvedValueOnce({ confirmPort: true }); const result = await askForCustomPort(); expect(result).toBe(1023); }); test('should handle port 1024 as non-reserved', async () => { jest .spyOn(inquirer, 'prompt') .mockResolvedValueOnce({ customPort: '1024' }); const result = await askForCustomPort(); expect(result).toBe(1024); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/setup/askForCustomPort/askForCustomPort.test.ts(1 hunks)src/setup/askForCustomPort/askForCustomPort.ts(1 hunks)
🔇 Additional comments (4)
src/setup/askForCustomPort/askForCustomPort.ts (2)
3-4: LGTM! Good choice of constants
The default port and retry attempts are well-defined with appropriate values. Using a non-reserved port (4321) as default is a good practice.
19-30: LGTM! Good safety defaults
The reserved port warning is well-implemented with a safe default (false), requiring explicit user confirmation.
src/setup/askForCustomPort/askForCustomPort.test.ts (2)
1-10: LGTM! Good test setup
The test setup is well-structured with proper mocking and cleanup between tests.
43-66: LGTM! Comprehensive retry testing
The retry mechanism tests thoroughly cover both successful recovery and maximum attempts scenarios.
|
@palisadoes I have added tests related to all functionalities of my code . The only job I'm failing is cause I changed |
palisadoes
merged commit 9108753
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
The version of 'inquirer' package has been upgraded to 11.0.2
Introduced Validation in
askForCustomPort.tsIssue Number:
Fixes #2294
Did you add tests for your changes?
I edited
src/setup/askForCustomPort/askForCustomPort.test.tsandsrc/setup/askForTalawaApiUrl/askForTalawaApiUrl.test.tsSnapshots/Videos:
If relevant, did you update the documentation?
No
Summary
http://localhost:4000/graphqlDoes this PR introduce a breaking change?
Not sure
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
inquirerpackage for potential improvements.Bug Fixes
Tests