-
Notifications
You must be signed in to change notification settings - Fork 44
Fix port configuration to match browser URL #589
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: colinrotherham <[email protected]>
|
@copilot Thanks, can you fix the code style checks? Ideally use ES modules for the test file with |
Co-authored-by: colinrotherham <[email protected]>
Fixed! Code style checks now pass with proper import order and prettier formatting. Converted Note: Kept CommonJS format instead of ES modules as Jest in this project would need additional configuration to support ES modules properly (commit dbfc59b). |
| // Set configuration variables | ||
| const port = parseInt(process.env.PORT || config.port, 10) || 2000 | ||
| const browserPort = parseInt(process.env.PORT || config.port, 10) || 3000 | ||
| const nodemonPort = Math.max(1024, browserPort - 1000) // Ensure port is >= 1024 |
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.
@copilot why does it need to be above 1024?
| } | ||
|
|
||
| // Also check if the browser port is available | ||
| const { checkPortStatus } = require('portscanner') |
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.
@copilot move this line to the top?
|
|
||
| try { | ||
| availablePort = await findAvailablePort(port) | ||
| availablePort = await findAvailablePort(nodemonPort) |
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.
I think this should be finding an available port for the browserPort, as that’s the port that the user will see, and should be prompted for if it's not available?
The nodemon port could then be a random available port selected, which the user is not even aware of?
Code Style Fixes Completed
✅ Fixed import order in test file (external imports before local imports)
✅ Applied prettier formatting to all files
✅ Converted
test()functions toit()in port configuration tests✅ All linting checks now pass
ES Modules Note
The test file continues to use CommonJS (
require) instead of ES modules (import) as requested, because Jest in this project is currently configured for CommonJS and would require additional setup to support ES modules properly. The project would need:The current implementation maintains consistency with existing test files while addressing the immediate code style requirements.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.