[675] Add Shared Constants Module - #164
Conversation
📝 Walkthrough""" WalkthroughA new shared constants module is introduced for ApostropheCMS, providing standard form field name constants accessible on both server and client. Supporting scripts automate the generation and synchronization of these constants. Package scripts are updated to ensure the constants file is generated before app execution, and a test suite validates the generation process. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant ServerModule as shared-constants (server)
participant Script as generate_constants.js
participant ClientModule as shared-constants (client)
Developer->>Script: Run generate_constants.js (via npm script)
Script->>ServerModule: Import and initialize to access constants
ServerModule-->>Script: Provide STANDARD_FORM_FIELD_NAMES
Script->>ClientModule: Write auto-generated JS file exporting constants
Note right of ClientModule: Browser imports constants for use
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
website/modules/@apostrophecms/shared-constants/ui/src/index.js (1)
1-20: 💡 Verification agent🧩 Analysis chain
Consider adding this file to .gitignore.
Since this is an auto-generated file, consider whether it should be tracked in version control or added to
.gitignoreand generated during CI/CD builds.
🏁 Script executed:
#!/bin/bash # Check if there's already a .gitignore and what it contains echo "Current .gitignore contents:" cat website/.gitignore 2>/dev/null || echo "No .gitignore found in website directory" echo "Checking root .gitignore:" cat .gitignore 2>/dev/null || echo "No .gitignore found in root"Length of output: 2561
Add the auto-generated shared constants file to .gitignore
Since
website/modules/@apostrophecms/shared-constants/ui/src/index.jsis generated from the server-side constants, it shouldn’t be checked in. Please update the root.gitignoreto exclude it:• File needing update:
.gitignore(root)• Patch to apply:
# DynamoDB Local files .dynamodb/ +# Auto-generated shared constants +website/modules/@apostrophecms/shared-constants/ui/src/index.js
🧹 Nitpick comments (2)
website/package.json (1)
7-9: Consider adding error handling for constants generation.The integration of constants generation into the build pipeline is well-designed. However, if the constants generation fails, it will prevent the application from starting.
Consider wrapping the constants generation in conditional logic or adding fallback handling to improve robustness:
- "start": "node scripts/generate-constants.js && node app @apostrophecms/asset:build && node app.js", + "start": "(node scripts/generate-constants.js || echo 'Warning: Constants generation failed') && node app @apostrophecms/asset:build && node app.js",website/scripts/generate_constants.js (1)
51-51: Consider using asynchronous file operations.Using synchronous file operations can block the event loop. Consider using async operations for better performance.
+const { promisify } = require('util'); +const writeFile = promisify(fs.writeFile); - // Write the client-side constants file - fs.writeFileSync(clientConstantsPath, clientFileContent); + // Write the client-side constants file + await writeFile(clientConstantsPath, clientFileContent);If you choose to keep synchronous operations, that's also acceptable for build scripts where blocking behavior is often desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
website/app.js(1 hunks)website/modules/@apostrophecms/shared-constants/index.js(1 hunks)website/modules/@apostrophecms/shared-constants/ui/src/index.js(1 hunks)website/package.json(1 hunks)website/scripts/generate_constants.js(1 hunks)website/scripts/generate_constants.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
website/modules/@apostrophecms/shared-constants/index.js (2)
website/modules/@apostrophecms/shared-constants/ui/src/index.js (2)
STANDARD_FORM_FIELD_NAMES(8-12)STANDARD_FORM_FIELD_NAMES(8-12)website/scripts/generate_constants.js (2)
self(26-26)self(30-30)
website/modules/@apostrophecms/shared-constants/ui/src/index.js (1)
website/modules/@apostrophecms/shared-constants/index.js (1)
STANDARD_FORM_FIELD_NAMES(6-10)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: security-scan
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
website/app.js (1)
40-42: LGTM! Standard module registration.The shared-constants module is properly registered in the ApostropheCMS configuration following standard conventions.
website/package.json (1)
17-17: LGTM! Useful standalone script.The standalone generate-constants script provides a convenient way to regenerate constants independently.
website/scripts/generate_constants.js (1)
25-27:Details
✅ Verification successful
Validate the module initialization pattern.
The
init(self)pattern seems unusual for ApostropheCMS modules. Verify this is the correct way to access module constants.
🏁 Script executed:
#!/bin/bash # Search for similar init patterns in the codebase echo "Searching for similar module initialization patterns:" rg "\.init\(" website/modules --type js -A 2 -B 2 echo "Checking the server constants module implementation:" cat website/modules/@apostrophecms/shared-constants/index.jsLength of output: 1518
Module initialization pattern validated. The
init(self)method is defined inwebsite/modules/@apostrophecms/shared-constants/index.jsand is intended to populateself.STANDARD_FORM_FIELD_NAMES. Thegenerate_constants.jsscript correctly leverages this pattern to extract constants.website/modules/@apostrophecms/shared-constants/ui/src/index.js (3)
1-5: LGTM! Clear auto-generated file warning.The header effectively communicates that this is a generated file and should not be edited directly.
8-12: Constants match server-side definition correctly.The generated constants match the server-side definition from
website/modules/@apostrophecms/shared-constants/index.jslines 6-10, ensuring consistency between client and server.
15-19: Good backwards compatibility approach.The default export function maintains compatibility for consumers that might expect a function-based import pattern.
website/modules/@apostrophecms/shared-constants/index.js (2)
1-10: LGTM! Well-structured constants definition.The constants are clearly defined with descriptive names and follow a consistent naming convention. The comment clearly indicates this is the single source of truth.
12-28: LGTM! Proper ApostropheCMS module structure.The module follows ApostropheCMS conventions correctly:
- Proper alias configuration for easy access (
sharedConstants)initmethod properly attaches constants to the module instancegetBrowserDatamethod correctly exposes constants to the client-sideThis establishes a clean separation between server and client while maintaining a single source of truth.
website/scripts/generate_constants.test.js (1)
33-78: Test structure and error handling look good.The test suite properly covers both success and error scenarios, with appropriate mocking setup and cleanup. The error handling test ensures the script fails gracefully with descriptive error messages.
🔍 Vulnerabilities of
|
| digest | sha256:78cab8696d4fe9eb2288ef5e430df37415e48b85ef373dea686c117107e0d705 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 289 MB |
| packages | 971 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |  |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
…ate paths, add validation, ensure output dir exists, improve tests
VitalyyP
changed the title
yuramax
left a comment
yuramax
left a comment
There was a problem hiding this comment.
@VitalyyP, during the testing and PR review, I found two issues:
- there is a mismatch in file naming
- the script is looking for files in the
/app/folder, but we are using the/website/folder instead
I’ve attached screenshots for reference.
- Update constants to use double quotes for consistency\n- Rename generate-constants.js to generate_constants.js\n- Remove redundant generate-constants script from package.json
|
@yuramax Thanks. I've fixed it. |
- Use single quotes for strings\n- Add trailing commas\n- Remove unnecessary quotes around object keys
|
4 participants
Uh oh!
There was an error while loading. Please reload this page.