[FE-Fix] Express 의존성 위치 변경 및 경로 에러 수정 #411
Conversation
WalkthroughThe changes update build scripts, TypeScript configurations, and deployment workflow for both client and server frontend apps. Scripts are refactored for direct use of built server output. TypeScript incremental builds are enabled, and type safety is improved in server code. The deployment pipeline is adjusted to organize build artifacts under a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as CI/CD Workflow
participant Build as Build System
participant Server as Server App
participant Client as Client App
participant EC2 as EC2 Instance
Dev->>CI: Push code / PR merged
CI->>Build: Run build scripts (pnpm -r build)
Build->>Client: Build client output to dist/
Build->>Server: Build server output to dist/
CI->>CI: Prepare dist/client and dist/server folders
CI->>EC2: Copy dist/ folder structure to EC2
EC2->>Server: Start server from dist/server/index.js
Server->>Client: Serve built client files from dist/client
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
hamo-o
changed the title
There was a problem hiding this comment.
각 폴더에 빌드 결과물을 생성하고, 이후 CD과정에서 처리해도 될 것 같긴 한데 고민이네요.
레퍼런스 찾기가 쉽지않네요.. ㅜㅜ 같이 고민해봐요!
제가 이해하기로 현재 문제되는 부분이
- cd 시에 루트에 있는 dist를 참조하게 됨
- ec2 인스턴스가 /dist/server를 실행할 때, 루트 기준으로 node가 실행되므로 express를 찾지 못함 (express는 /apps/server에 설치돼 있기 때문)
- 이를 해결하기 위해 /apps/server에 정의되어있던 express 의존성을 루트로 이전
인 것 같은데요,
제가 이해한 바가 맞다면, 현영님이 말씀하신 "각 폴더에 빌드 결과물을 생성하고, 이후 CD과정에서 처리" 하는 방식이 제 생각엔 더 나아 보이긴 합니다..!
frontend/package.json
Outdated
| "version": "0.0.0", | ||
| "type": "module", | ||
| "scripts": { | ||
| "build:server": "pnpm vite build apps/server", |
There was a problem hiding this comment.
혹시 서버 빌드 스크립트를 server가 아닌 frontend의 package.json에 정의하신 이유가 있으신가요? pnpm -r build를 실행하면 모든 워크스페이스의 build 스크립트가 실행되는데, 현재 frontend에 정의된 build:server 를 server에다가 build 로 정의해도 되지 않을까 싶어서요!
+ 아 혹시 아래 말씀하신 내용 때문에 서버 빌드 스크립트를 루트에 정의하신 걸까요?
현재 서버에서는 루트에서 빌드 결과물을 실행하기 때문에 런타임인 express 역시 해당 경로에 있어야 정상 실행되어, 경로를 바꾸었습니다. (package.json을 복사하고, 이후 설치하는 방법도 있다고 하는데 일반적이진 않은 것 같아요.)
There was a problem hiding this comment.
요건 vite 때문이었습니다..! 각 폴더에 빌드 결과물 생성 후 실행시키려면 스크립트를 꽤 바꿔야 할 것 같아 고민해보겠습니다 의견 감사합니다!
|
다음 프로세스로 변경했습니다.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/apps/client/tsconfig.node.json (1)
4-4: Consider excluding tsbuildinfo files from version control
ThetsBuildInfoFileundernode_modules/.tmpmay clutter your repo—ensure you’ve added this path to.gitignoreor a dedicated ignore file.frontend/apps/server/tsconfig.json (2)
11-11: Enable incremental compilation
Incremental builds will generate a default.tsbuildinfoin the project root—consider specifying a customtsBuildInfoFile(e.g.,"./node_modules/.tmp/tsconfig.server.tsbuildinfo") for consistency with other configs.
2-15: AddrootDirto ensure correct source mapping
Explicitly setting"rootDir": "src"prevents stray files outsidesrcfrom being compiled.{ "compilerOptions": { "target": "ESNext", "module": "ESNext", "moduleResolution": "node", "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "strict": true, "skipLibCheck": true, "outDir": "dist", "incremental": true, + "rootDir": "src" }, "include": ["src"], "exclude": ["node_modules", "dist", "**/*.test.ts"] }frontend/apps/client/tsconfig.app.json (1)
17-17: Enable incremental builds for app compilation
Incremental compilation will leverage previous build info. Remember to ignore the.tsbuildinfofile in version control.frontend/apps/server/src/cookies.ts (1)
2-6: Consider defensive programming for malformed cookie strings.The current implementation assumes well-formed cookie strings. Consider handling edge cases where cookies might be malformed (missing '=' separator or empty cookie strings).
const cookieStore = cookieHeader.split('; ').reduce((acc, cookie) => { - const [key, value] = cookie.split('='); - acc[key] = decodeURIComponent(value); + const [key, value] = cookie.split('='); + if (key && value !== undefined) { + acc[key] = decodeURIComponent(value || ''); + } return acc; }, {} as Record<string, string>);.github/workflows/fe-cd.yml (1)
23-23: Minor style inconsistency with quotes.The change from single quotes to double quotes for the node-version is cosmetic but creates inconsistency with other string values in the workflow file.
Consider keeping consistent quote usage throughout the file. If changing to double quotes, update other string values for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/fe-cd.yml(3 hunks)frontend/apps/client/package.json(1 hunks)frontend/apps/client/tsconfig.app.json(1 hunks)frontend/apps/client/tsconfig.node.json(1 hunks)frontend/apps/client/vite.config.ts(1 hunks)frontend/apps/server/package.json(1 hunks)frontend/apps/server/src/cookies.ts(1 hunks)frontend/apps/server/src/index.ts(2 hunks)frontend/apps/server/tsconfig.json(1 hunks)frontend/apps/server/tsup.config.ts(1 hunks)frontend/package.json(1 hunks)frontend/tsconfig.json(1 hunks)
🔇 Additional comments (22)
frontend/apps/client/tsconfig.node.json (1)
3-3: Enable incremental builds for Node environment
Enablingincrementalspeeds up subsequent compilations by reusing previous build information.frontend/tsconfig.json (2)
8-9: Update project reference paths
Renamed"packages/endolphinCalendar"to"packages/calendar"and"packages/endolphinDateTime"to"packages/date-time". This aligns with the new folder structure.
8-9:Details
✅ Verification successful
Verify removal of old references
Ensure no lingering import or build references to the old paths remain elsewhere in the monorepo.
🏁 Script executed:
#!/bin/bash # Search for outdated package paths rg -l "endolphinCalendar" -l "endolphinDateTime"Length of output: 104
🏁 Script executed:
#!/bin/bash # Search for lingering old package identifiers throughout the repo rg -l -e 'endolphinCalendar' . rg -l -e 'endolphinDateTime' .Length of output: 61
No lingering old package references found
Verified by searching the entire repo forendolphinCalendarandendolphinDateTime—no occurrences remain.All clear to proceed.
frontend/apps/client/tsconfig.app.json (1)
10-10: Approve new path alias for components
Adding@components/*aligns with existing aliases and improves import consistency.frontend/apps/client/vite.config.ts (2)
16-16: Explicit outDir configuration is clear but redundant.While explicitly setting
outDir: 'dist'improves configuration clarity and aligns with the PR's build output organization goals, note that 'dist' is already Vite's default output directory. The explicit configuration is helpful for documentation purposes.
19-24: Good formatting cleanup.The removal of trailing spaces in the TanStackRouterVite plugin configuration improves code consistency.
frontend/apps/server/package.json (3)
8-9: Well-structured build and start scripts.The new scripts using
tsupfor building and the watch-based start script align perfectly with the PR's objective of improving build output handling. The--onSuccesscallback ensures the server runs only after successful compilation.
12-12:Details
✅ Verification successful
Verify Express v5 compatibility and breaking changes.
The upgrade from Express v4.21.2 to v5.1.0 is a major version jump that may introduce breaking changes. Ensure the server code is compatible with Express v5 changes.
🏁 Script executed:
#!/bin/bash # Description: Check for Express v5 usage patterns and potential compatibility issues # Check how Express is used in the server code rg -A 5 -B 2 "express\(" frontend/apps/server/src/ rg -A 5 -B 2 "from ['\"]express['\"]" frontend/apps/server/src/ rg -A 5 -B 2 "require\(['\"]express['\"]" frontend/apps/server/src/Length of output: 1176
Express v5 Compatibility Verified
- Only import of Express is in
frontend/apps/server/src/index.ts:
•import express from 'express'
•const app = express()- No deprecated v4 patterns detected (e.g.
res.send(status, body), standalonebody-parser, customexpress.Routeroverloads).The upgrade to Express 5.1.0 appears safe. Please run your full test suite and smoke-test all endpoints to confirm.
14-17:Details
✅ Verification successful
Verify @types/express version alignment with Express v5.
The
@types/expressversion ^5.0.2 should be compatible with Express ^5.1.0, but it's worth verifying that the type definitions are up-to-date and accurate for the Express version being used.
🌐 Web query:
Are @types/express version 5.0.2 and Express version 5.1.0 compatible? What are the main breaking changes in Express v5?💡 Result:
Yes,
@types/expressversion 5.0.2 is compatible with Express version 5.1.0. The@types/expresspackage provides TypeScript type definitions for Express, and version 5.0.2 aligns with the Express 5.x series, ensuring compatibility with Express 5.1.0.Express v5 introduced several breaking changes compared to previous versions. Here are the main changes:
Node.js Version Support: Express v5 drops support for Node.js versions prior to v18, requiring developers to upgrade their Node.js environments. (expressjs.com)
Path Route Matching Syntax: The routing system has been updated to enhance security and clarity. Notably, sub-expression regular expressions (e.g.,
/:param(\\d+)) are no longer supported due to potential security vulnerabilities. Additionally, the syntax for optional parameters has changed; for example,:param?is now written as{:param}. (expressjs.com)Promise Support: Express v5 introduces improved handling of promises in middleware and route handlers. If an asynchronous function returns a rejected promise, Express will automatically pass the error to the error-handling middleware, streamlining error management. (expressjs.com)
Body Parser Changes: The default behavior of the
express.urlencoded()middleware has been modified. Theextendedoption now defaults tofalse, which affects how nested objects are parsed. Additionally, the depth of nested objects is limited to 32 by default, addressing security concerns. (expressjs.com)Removal of Deprecated Methods: Several methods and properties that were deprecated in earlier versions have been removed in Express v5, including:
app.del()app.param(fn)req.param(name)res.sendfile()res.json(obj, status)res.jsonp(obj, status)res.redirect(url, status)res.send(body, status)res.send(status)req.acceptsCharset()req.acceptsEncoding()req.acceptsLanguage()res.clearCookie()res.status()(now enforces stricter validation)res.vary()(now enforces stricter validation)Developers should update their code to remove reliance on these methods and properties to ensure compatibility with Express v5. (expressjs.com)
For a comprehensive list of breaking changes and detailed migration instructions, refer to the official Express migration guide. (expressjs.com)
Citations:
- 1: https://expressjs.com/2024/10/15/v5-release?utm_source=openai
- 2: https://expressjs.com/2024/10/15/v5-release?utm_source=openai
- 3: https://expressjs.com/2024/10/15/v5-release?utm_source=openai
- 4: https://expressjs.com/2024/10/15/v5-release?utm_source=openai
- 5: https://expressjs.com/en/guide/migrating-5.html?utm_source=openai
- 6: https://expressjs.com/en/guide/migrating-5.html?utm_source=openai
Verified @types/express Compatibility with Express v5.1.0
Confirmed that
@types/express@^5.0.2is fully compatible with Express v5.1.0. No changes are needed infrontend/apps/server/package.json.Main breaking changes in Express v5 to be aware of:
- Requires Node.js v18 or newer
- Updated route‐matching syntax (no sub-expression regex,
:param?→{:param})- Built-in Promise support in middleware and handlers
- Changed
express.urlencoded()defaults (extended: false, max depth 32)- Removal of legacy/deprecated APIs (e.g.,
app.del(),res.sendfile(),req.param())For full migration details, see the official guide:
https://expressjs.com/en/guide/migrating-5.htmlfrontend/package.json (1)
7-8: Good monorepo script organization with recursive pnpm commands.The recursive
pnpm -r buildandpnpm -r devcommands are well-suited for the monorepo structure, allowing each workspace to define its own build and dev scripts while providing convenient root-level commands. This approach addresses the earlier discussion about script placement by maintaining clear separation of concerns.frontend/apps/server/src/cookies.ts (2)
6-6: Good explicit typing for type safety.The explicit
Record<string, string>typing for the reduce accumulator improves type safety and makes the intended data structure clear.
8-8: Explicit parameter typing improves function signature clarity.Adding the
stringtype annotation to thekeyparameter enhances type safety and function documentation.frontend/apps/server/tsup.config.ts (1)
1-12: LGTM! Configuration aligns well with PR objectives.The tsup configuration correctly supports the build restructuring mentioned in the PR objectives. The
bundle: falsesetting ensures Express and other dependencies remain separate, which aligns with the need to have Express in the correct location for runtime execution..github/workflows/fe-cd.yml (3)
61-68: Excellent restructuring that aligns with PR objectives.This new step correctly implements the build output restructuring mentioned in the PR objectives. By creating a unified
frontend/dist/structure and copying the server'spackage.json, it ensures Express dependencies can be properly installed in the deployment location.
80-89: Proper dependency installation in the correct location.This step correctly installs dependencies in the
frontend/dist/serverdirectory, which aligns with the PR objective of ensuring Express is available in the runtime location.
66-67:Details
✅ Verification successful
Verify that the source directories exist after build.
Ensure that
frontend/apps/client/dist/andfrontend/apps/server/dist/directories are created by the build process before this copy step runs.
🏁 Script executed:
#!/bin/bash # Description: Verify build scripts create the expected dist directories # Expected: Find build scripts that output to dist/ in both client and server apps echo "Checking client build configuration..." fd -t f "vite.config" frontend/apps/client/ --exec cat {} echo -e "\nChecking server build configuration..." fd -t f "tsup.config" frontend/apps/server/ --exec cat {} echo -e "\nChecking package.json build scripts..." cat frontend/apps/client/package.json | jq '.scripts.build' cat frontend/apps/server/package.json | jq '.scripts.build'Length of output: 2273
Build output directories verified – no changes required
Both the client and server builds produce their respective
distdirectories:
- Client:
vite.configsetsoutDir: 'dist', matchingfrontend/apps/client/dist/- Server:
tsup.configoutputs todist, matchingfrontend/apps/server/dist/The existing
cpsteps will succeed as-is.frontend/apps/server/src/index.ts (6)
8-8: Good type safety improvement.Adding explicit typing for
ViteDevServerimproves code clarity and IDE support.
15-17: Excellent path consolidation that supports the new build structure.The
clientPathvariable with conditional logic correctly addresses the path differences between production (built output) and development environments. This aligns perfectly with the PR objectives of fixing path errors.
21-21: Good explicit typing for better type safety.Adding explicit typing for the
vitevariable improves code clarity and type checking.
62-66: Improved error handling with proper type guards.The addition of type checking with
e instanceof Errorbefore accessing error properties is a good defensive programming practice that prevents runtime errors.
83-85: Enhanced console formatting for better readability.The multiline console.log formatting with trailing commas improves code readability and follows modern JavaScript practices.
Also applies to: 90-92
45-45:Details
❓ Verification inconclusive
Verify route pattern change behavior.
The route pattern changed from a wildcard string
'*'to a regex/(.*)/. While functionally similar, this might have subtle behavioral differences.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any specific routing requirements or tests that depend on the exact route pattern # Expected: Find tests or documentation about routing behavior echo "Searching for route-related tests..." rg -A 5 -B 5 "route|middleware" frontend/apps/server/src/ || echo "No route tests found" echo -e "\nSearching for route patterns in related files..." rg "\*|/\(\.\*\)/" frontend/apps/ || echo "No similar patterns found"Length of output: 9679
Verify catch-all route regex behavior
I didn’t find any existing tests or docs tied to the old
'*'wildcard in frontend/apps/server/src/index.ts. Switching to the regex/(.*)/will broadly catch all routes, but Express’s string vs. RegExp matching can differ in subtle ways. Please double-check:
- That nested paths (e.g.
/foo/bar) are still handled as before.- That
express.static(…)continues to take precedence for asset requests.- That missing‐file requests don’t inadvertently fall back to your SPA handler.
Consider adding a lightweight integration test to assert both asset serving and SPA fallback behavior.
Location: frontend/apps/server/src/index.ts, line 45
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
express역시 해당 경로에 있어야 정상 실행되어, 경로를 바꾸었습니다. (package.json을 복사하고, 이후 설치하는 방법도 있다고 하는데 일반적이진 않은 것 같아요.)🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
각 폴더에 빌드 결과물을 생성하고, 이후 CD과정에서 처리해도 될 것 같긴 한데 고민이네요.
레퍼런스 찾기가 쉽지않네요.. ㅜㅜ 같이 고민해봐요!
Summary by CodeRabbit
New Features
Improvements
Chores