feature: added serving capability to static pages #37
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds serving capability to static pages by porting the applitools.github.io demo server for CI usage. The implementation provides a TypeScript-based Express server that can serve demo website content locally, enabling CI environments to host the demo pages for testing purposes.
- Implements a DemoServer class with static serving functionality and configurable options
- Adds CLI interfaces for both programmatic usage and command-line execution
- Includes comprehensive documentation and build configuration for TypeScript compilation
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server.ts | Core server implementation with Express-based static file serving |
| src/test-server.ts | Test script demonstrating server functionality |
| src/server-cli.ts | CLI interface with argument parsing for flexible server configuration |
| package.json | Project configuration with dependencies and build scripts |
| rollup.config.js | Rollup bundling configuration for deployment |
| README.md | Comprehensive documentation covering API usage and project structure |
| EXTERNAL_DEPENDENCIES.md | Documentation of external resources used by served pages |
| .eslintrc.json | ESLint configuration for TypeScript code quality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| export default DemoServer; | ||
|
|
||
| // If this file is run directly, start the server | ||
| if (import.meta.url === `file://${process.argv[1]}`) { |
There was a problem hiding this comment.
The condition for detecting if the file is run directly is incorrect. process.argv[1] contains the script path, but import.meta.url is a file URL. Use fileURLToPath(import.meta.url) === process.argv[1] instead.
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| if (__filename === process.argv[1]) { |
| const usedIndices = new Set<number>(); | ||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i] === '--port' || args[i] === '-p') { | ||
| port = parseInt(args[i + 1], 10) || 3000; |
There was a problem hiding this comment.
Using || 3000 after parseInt will default to 3000 even for valid port 0. Use isNaN(parsed) ? 3000 : parsed pattern instead to properly handle edge cases.
| port = parseInt(args[i + 1], 10) || 3000; | |
| { | |
| const parsed = parseInt(args[i + 1], 10); | |
| port = isNaN(parsed) ? 3000 : parsed; | |
| } |
| outputToFilesystem: false | ||
| }) | ||
| ], | ||
| external: ['express'] |
There was a problem hiding this comment.
Express is marked as external but the banner suggests this is meant to be an executable. Consider whether express should be bundled or if the executable approach conflicts with the external dependency.
| external: ['express'] |
There was a problem hiding this comment.
do we want to commit this document? Do you plan to remove those external deps?
| <style> | ||
| body { | ||
| font-family: Arial, sans-serif; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 100vh; | ||
| margin: 0; | ||
| background-color: #f5f5f5; | ||
| } | ||
| .error-container { | ||
| text-align: center; | ||
| padding: 2rem; | ||
| background: white; | ||
| border-radius: 8px; | ||
| box-shadow: 0 2px 4px rgba(0,0,0,0.1); | ||
| } | ||
| h1 { color: #d9534f; } | ||
| p { color: #666; } | ||
| </style> |
There was a problem hiding this comment.
this pr has a lot of code we don't actually need
especially since this is just a test server, let's keep things lean
Porting the applitools.github.io demo server for CI usage.
In order to serve the demo website in CI it can now be served via spinning a local server.