feat: generate destination constants #1800
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (6)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request introduces a new script in the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1800 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…erlabs/rudder-integrations-config into feat/generate-destination-constants
koladilip
requested review from
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020,
RanjeetMishra,
lvrach and
a team
as code owners
![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 and nitpick comments (2)
scripts/generateConstants.js (2)
1-7: Consider modernizing the code with ES modules and TypeScriptThe code currently uses CommonJS require statements. Consider modernizing by:
- Using ES modules (import/export)
- Adding TypeScript for better type safety and maintainability
-const fs = require('fs'); -const path = require('path'); +import * as fs from 'fs'; +import * as path from 'path';🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
9-14: Consider enhancing language mapping configurationThe current hardcoded language mapping could be improved by:
- Moving it to a separate configuration file for easier maintenance
- Adding type definitions for platforms and language codes
- Adding validation for supported language codes
Consider creating a separate
config.tsfile:export interface PlatformConfig { [platform: string]: string[]; } export const languageMap: PlatformConfig = { web: ["ts"], flutter: ["dart"], ios: ["java", "m", "swift"], android: ["kt", "java"] };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
generated/Destinations.dartis excluded by!**/generated/**generated/Destinations.javais excluded by!**/generated/**generated/Destinations.ktis excluded by!**/generated/**generated/Destinations.mis excluded by!**/generated/**generated/Destinations.swiftis excluded by!**/generated/**generated/Destinations.tsis excluded by!**/generated/**generated/destinations.tsis excluded by!**/generated/**
📒 Files selected for processing (9)
package.json(2 hunks)scripts/generateConstants.js(1 hunks)templates/Destinations.dart.template(1 hunks)templates/Destinations.java.template(1 hunks)templates/Destinations.kt.template(1 hunks)templates/Destinations.m.template(1 hunks)templates/Destinations.swift.template(1 hunks)templates/Destinations.ts.template(1 hunks)templates/destinations.ts.template(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- templates/Destinations.dart.template
- templates/Destinations.java.template
- templates/Destinations.kt.template
- templates/Destinations.m.template
- templates/Destinations.swift.template
- templates/Destinations.ts.template
- templates/destinations.ts.template
🧰 Additional context used
🪛 eslint
scripts/generateConstants.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (2)
package.json (2)
41-41: LGTM: Valid JSON syntax maintained
The added comma is syntactically correct and maintains valid JSON structure.
42-42: Verify the generate:constants script file
The script addition aligns with the PR objectives for generating destination constants.
Let's verify the script file exists and check its basic structure:
✅ Verification successful
Let me gather more information about the destinations directory and templates to ensure completeness of the verification.
Let me check the content of one of the template files to verify the template structure and its relationship with the destinations.
Based on the comprehensive verification results, I can now provide the final response:
✓ The generateConstants script is properly structured and functional
The script is well-implemented with:
- Correct directory structure (templates, generated, destinations)
- Proper template processing logic that reads from
src/configurations/destinations - Generation of constants for multiple languages (TS, Java, Swift, Dart, etc.)
- Safe file operations with proper error handling
The script successfully:
- Reads destination configurations from
db-config.jsonfiles - Filters destinations based on language support
- Processes templates to generate language-specific constants
- Outputs files to the
generateddirectory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the generateConstants.js script exists and contains expected content
# Check if the script file exists
if [ -f "scripts/generateConstants.js" ]; then
echo "✓ Script file exists"
# Check if it contains basic template processing logic
rg -A 5 "template" "scripts/generateConstants.js"
else
echo "✗ Script file not found at scripts/generateConstants.js"
fi
Length of output: 2329
Script:
#!/bin/bash
# Check the existence and structure of related directories and files
# Check templates directory
echo "=== Templates Directory ==="
if [ -d "templates" ]; then
echo "✓ Templates directory exists"
echo "Template files:"
ls -la templates/
fi
# Check generated directory
echo -e "\n=== Generated Directory ==="
if [ -d "generated" ]; then
echo "✓ Generated directory exists"
echo "Generated files:"
ls -la generated/
fi
# Check destinations directory
echo -e "\n=== Destinations Directory ==="
if [ -d "src/configurations/destinations" ]; then
echo "✓ Destinations directory exists"
echo "Destination files:"
ls -la src/configurations/destinations/
fi
# Check for any existing template files
echo -e "\n=== Template Files ==="
fd -e template
Length of output: 16668
Script:
#!/bin/bash
# Check template content and structure
# Check a template file content
echo "=== Template Content Example ==="
cat templates/Destinations.ts.template
# Check if the script processes all destination directories
echo -e "\n=== Script Processing Check ==="
rg -A 5 "destinations" scripts/generateConstants.js
Length of output: 2031
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
scripts/generateConstants.js (2)
52-60: Use regex for more robust filename parsingConsider using a regex pattern for more reliable filename parsing.
function getLangCode(templateFileName) { - const parts = templateFileName.split('.'); - if (parts.length === 3) { - return parts[1]; - } - throw new Error(`Invalid template file name: ${templateFileName}`); + const match = templateFileName.match(/^[^.]+\.([^.]+)\.template$/); + if (!match) { + throw new Error( + `Invalid template filename: ${templateFileName}. ` + + 'Expected format: <template-name>.<lang-code>.template' + ); + } + return match[1]; }
62-90: Add basic error handling for critical file operationsWhile internal files are trusted, basic error handling for critical file operations would improve reliability.
function generateFiles() { + try { // Ensure the 'generated' directory exists if (!fs.existsSync(generatedDir)) { fs.mkdirSync(generatedDir); } // Read all files in the templates directory fs.readdirSync(templatesDir) .filter((file) => file.endsWith('.template')) .forEach((file) => { + try { const filePath = path.join(templatesDir, file); const destinations = prepareDestinations(getLangCode(file)); const templateContent = fs.readFileSync(filePath, 'utf8'); const output = processTemplate(templateContent, destinations); const outputFilePath = path.join(generatedDir, file.replace('.template', '')); fs.writeFileSync(outputFilePath, output); console.log(`Generated: ${outputFilePath}`); + } catch (error) { + console.error(`Failed to generate ${file}: ${error.message}`); + } }); + } catch (error) { + console.error(`Generation failed: ${error.message}`); + process.exit(1); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/generateConstants.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/generateConstants.js (2)
Learnt from: koladilip
PR: rudderlabs/rudder-integrations-config#1800
File: scripts/generateConstants.js:51-60
Timestamp: 2024-11-18T15:30:16.794Z
Learning: In internal Node.js scripts like `scripts/generateConstants.js`, where file validity is assured, additional error handling and async operations are not required.
Learnt from: koladilip
PR: rudderlabs/rudder-integrations-config#1800
File: scripts/generateConstants.js:39-44
Timestamp: 2024-11-18T15:29:10.961Z
Learning: In `scripts/generateConstants.js`, the `processTemplate` function uses `new Function()` but since the template is not taking input from outside, it's acceptable to use `new Function()` in this context.
🪛 eslint
scripts/generateConstants.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (4)
scripts/generateConstants.js (4)
1-8: LGTM! Path setup follows Node.js best practices
The path construction using __dirname ensures reliable file resolution regardless of the working directory.
🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
34-39: LGTM! Template processing is appropriate for internal use
The use of new Function() is acceptable here as the templates are internal and not user-provided.
41-50: LGTM! Destination preparation is straightforward
The function efficiently processes destination configurations and extracts required properties.
92-93: LGTM! Script execution is straightforward
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
scripts/generateConstants.js (3)
47-56: Add type validation for destination propertiesWhile error handling isn't required for internal files, adding type validation would make the code more robust.
Consider adding type checks:
function prepareDestinations(langCode) { return fs.readdirSync(destinationsDir) .map((destination) => { const destinationsFilePath = path.join(destinationsDir, destination, "db-config.json"); const destinationsContent = fs.readFileSync(destinationsFilePath, 'utf8'); - return JSON.parse(destinationsContent); + const config = JSON.parse(destinationsContent); + if (typeof config.name !== 'string' || typeof config.displayName !== 'string') { + console.warn(`Destination ${destination} has invalid name or displayName`); + return null; + } + return config; }) + .filter(Boolean) .filter((destination) => filterLanguages(destination, langCode)) .map((destination) => ({ name: destination.name, displayName: destination.displayName })); }
58-67: Use regex for more robust filename validationThe current string splitting approach could be made more robust using regex validation.
Consider using regex for validation:
function getLangCode(templateFileName) { - const parts = templateFileName.split('.'); - if (parts.length === 3) { - return parts[1]; - } - throw new Error(`Invalid template file name: ${templateFileName}`); + const match = templateFileName.match(/^[^.]+\.([^.]+)\.template$/); + if (!match) { + throw new Error( + `Invalid template filename: ${templateFileName}. ` + + 'Expected format: <template-name>.<lang-code>.template' + ); + } + return match[1]; }
69-96: Add summary logging for better monitoringConsider adding summary logging to track the overall generation process.
Add summary logging:
function generateFiles() { + const results = { total: 0, success: 0, skipped: 0 }; if (!fs.existsSync(generatedDir)) { fs.mkdirSync(generatedDir); } fs.readdirSync(templatesDir) .filter((file) => file.endsWith('.template')) .forEach((file) => { + results.total++; const filePath = path.join(templatesDir, file); const destinations = prepareDestinations(getLangCode(file)); + if (destinations.length === 0) { + console.log(`Skipped ${file}: No matching destinations`); + results.skipped++; + return; + } const templateContent = fs.readFileSync(filePath, 'utf8'); const output = processTemplate(templateContent, destinations); const outputFilePath = path.join(generatedDir, file.replace('.template', '')); fs.writeFileSync(outputFilePath, output); console.log(`Generated: ${outputFilePath}`); + results.success++; }); + console.log(`Generation complete: ${results.success}/${results.total} files generated, ${results.skipped} skipped`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
scripts/generateConstants.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/generateConstants.js (2)
Learnt from: koladilip
PR: rudderlabs/rudder-integrations-config#1800
File: scripts/generateConstants.js:51-60
Timestamp: 2024-11-18T15:30:16.794Z
Learning: In internal Node.js scripts like `scripts/generateConstants.js`, where file validity is assured, additional error handling and async operations are not required.
Learnt from: koladilip
PR: rudderlabs/rudder-integrations-config#1800
File: scripts/generateConstants.js:39-44
Timestamp: 2024-11-18T15:29:10.961Z
Learning: In `scripts/generateConstants.js`, the `processTemplate` function uses `new Function()` but since the template is not taking input from outside, it's acceptable to use `new Function()` in this context.
🪛 eslint
scripts/generateConstants.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (2)
scripts/generateConstants.js (2)
17-37: LGTM! Well-structured filtering logic
The function correctly:
- Validates required fields with appropriate warning logs
- Filters destinations based on connection modes
- Checks platform language support
40-45: LGTM! Template processing is appropriate for internal use
The implementation using new Function() is acceptable here as the templates are internal and not user-provided.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scripts/generateConstants.js (3)
1-9: Consider migrating to TypeScript for better type safetyThe script could benefit from TypeScript's type safety and modern ES module syntax. This would help catch potential errors early and improve maintainability.
Example migration:
import * as fs from 'fs'; import * as path from 'path'; // Define types for better maintainability interface PathConfig { templatesDir: string; generatedDir: string; destinationsDir: string; } const paths: PathConfig = { templatesDir: path.join(__dirname, '../templates'), generatedDir: path.join(__dirname, '../generated'), destinationsDir: path.join(__dirname, '../src/configurations/destinations'), };🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
42-52: Add basic error handling for file operationsWhile these are internal files, basic error handling would help diagnose issues during development.
function prepareDestinations(langCode) { + try { return fs .readdirSync(destinationsDir) .map((destination) => { const destinationsFilePath = path.join(destinationsDir, destination, 'db-config.json'); const destinationsContent = fs.readFileSync(destinationsFilePath, 'utf8'); return JSON.parse(destinationsContent); }) .filter((destination) => filterLanguages(destination, langCode)) .map((destination) => ({ name: destination.name, displayName: destination.displayName })); + } catch (error) { + console.error(`Error preparing destinations: ${error.message}`); + process.exit(1); + } }
65-91: Add error handling for file operations in generateFilesThe function should handle potential file operation errors gracefully.
function generateFiles() { + try { // Ensure the 'generated' directory exists if (!fs.existsSync(generatedDir)) { fs.mkdirSync(generatedDir); } fs.readdirSync(templatesDir) .filter((file) => file.endsWith('.template')) .forEach((file) => { + try { const filePath = path.join(templatesDir, file); const destinations = prepareDestinations(getLangCode(file)); const templateContent = fs.readFileSync(filePath, 'utf8'); const output = processTemplate(templateContent, destinations); const outputFilePath = path.join(generatedDir, file.replace('.template', '')); fs.writeFileSync(outputFilePath, output); console.log(`Generated: ${outputFilePath}`); + } catch (error) { + console.error(`Error processing ${file}: ${error.message}`); + } }); + } catch (error) { + console.error(`Generation failed: ${error.message}`); + process.exit(1); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
generated/Destinations.tsis excluded by!**/generated/**
📒 Files selected for processing (3)
package.json(1 hunks)scripts/generateConstants.js(1 hunks)templates/Destinations.ts.template(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- templates/Destinations.ts.template
🧰 Additional context used
🪛 eslint
scripts/generateConstants.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (1)
scripts/generateConstants.js (1)
17-33: LGTM! Well-structured filtering logic with proper error handling
The function correctly:
- Checks for missing supportedConnectionModes with appropriate logging
- Filters based on device/hybrid mode support
- Maps platforms to supported languages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
templates/Destinations.kt.template (2)
1-7: Fix typo and clarify file reference in header comment
- Line 6: "Containly" should be "Contains"
- Line 4: The URL reference might be confusing as it points to the generated file while this is the template file
- NOTE: Containly only constants for device mode destinations + NOTE: Contains only constants for device mode destinations
1-14: Consider enhancing the template generation systemTo improve the robustness and maintainability of the constants generation system, consider:
- Adding TypeScript interfaces for the destination data structure
- Implementing a validation step for the generated code (e.g., using a Kotlin parser)
- Using a template engine that supports proper code formatting
- Adding unit tests for the template generation process
This would help catch errors early and ensure consistent output across all language templates.
scripts/generateConstants.js (5)
1-9: Consider migrating to ES modulesThe script currently uses CommonJS require statements. While this works, migrating to ES modules would align with modern JavaScript practices and enable better TypeScript integration.
-const fs = require('fs'); -const path = require('path'); +import * as fs from 'fs'; +import * as path from 'path';🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
42-52: Add type validation for destination propertiesWhile error handling isn't required for internal files, adding type validation would make the code more robust.
.map((destination) => { + if (typeof destination.name !== 'string' || typeof destination.displayName !== 'string') { + console.warn(`Destination ${destination.name} has invalid name or displayName`); + return null; + } return { name: destination.name, displayName: destination.displayName }; -}); +}) +.filter(Boolean);
54-62: Enhance filename validation with regexThe current string splitting approach could be made more robust with a regex pattern.
function getLangCode(templateFileName) { - const parts = templateFileName.split('.'); - if (parts.length === 3) { - return parts[1]; - } - throw new Error(`Invalid template file name: ${templateFileName}`); + const match = templateFileName.match(/^[^.]+\.([^.]+)\.template$/); + if (!match) { + throw new Error( + `Invalid template filename: ${templateFileName}. ` + + 'Expected format: <template-name>.<lang-code>.template' + ); + } + return match[1]; }
64-91: Add generation statisticsConsider tracking and reporting generation statistics for better visibility.
function generateFiles() { + const stats = { total: 0, success: 0, skipped: 0 }; if (!fs.existsSync(generatedDir)) { fs.mkdirSync(generatedDir); } - fs.readdirSync(templatesDir) + const templates = fs.readdirSync(templatesDir) .filter((file) => file.endsWith('.template')) - .forEach((file) => { + stats.total = templates.length; + + templates.forEach((file) => { const filePath = path.join(templatesDir, file); const destinations = prepareDestinations(getLangCode(file)); + + if (destinations.length === 0) { + console.log(`Skipped: ${file} (no matching destinations)`); + stats.skipped++; + return; + } const templateContent = fs.readFileSync(filePath, 'utf8'); const output = processTemplate(templateContent, destinations); const outputFilePath = path.join(generatedDir, file.replace('.template', '')); fs.writeFileSync(outputFilePath, output); console.log(`Generated: ${outputFilePath}`); + stats.success++; }); + console.log(`\nGeneration complete: ${stats.success}/${stats.total} files generated, ${stats.skipped} skipped`); }
93-94: Add proper exit code handlingConsider adding proper exit code handling for better integration with build tools.
-// Generate the files -generateFiles(); +try { + generateFiles(); + process.exit(0); +} catch (error) { + console.error('Generation failed:', error); + process.exit(1); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
generated/Destinations.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
scripts/generateConstants.js(1 hunks)templates/Destinations.dart.template(1 hunks)templates/Destinations.java.template(1 hunks)templates/Destinations.kt.template(1 hunks)templates/Destinations.m.template(1 hunks)templates/Destinations.swift.template(1 hunks)templates/Destinations.ts.template(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- templates/Destinations.dart.template
- templates/Destinations.java.template
- templates/Destinations.m.template
- templates/Destinations.swift.template
- templates/Destinations.ts.template
🧰 Additional context used
🪛 eslint
scripts/generateConstants.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (3)
templates/Destinations.kt.template (1)
9-14: Enhance template robustness and formatting
The template could benefit from additional safety measures and formatting improvements:
- Consider adding null/undefined checks for dest.name and dest.displayName
- Add validation for special characters in displayName that might need escaping
- Ensure consistent formatting in the generated code
Consider updating the template to include validation and proper formatting:
object Destinations {
- ${destinations.map((dest) => `
- const val ${dest.name}_NAME = "${dest.name}"
-
- const val ${dest.name}_DISPLAY_NAME = "${dest.displayName}"`).join('\n')}
+ ${destinations.map((dest) => {
+ if (!dest.name || !dest.displayName) {
+ throw new Error(`Invalid destination: ${JSON.stringify(dest)}`);
+ }
+ const escapedDisplayName = dest.displayName.replace(/"/g, '\\"');
+ return `
+ const val ${dest.name}_NAME = "${dest.name}"
+ const val ${dest.name}_DISPLAY_NAME = "${escapedDisplayName}"
+ `;
+ }).join('\n').trim()}
}Let's verify the destinations data structure:
scripts/generateConstants.js (2)
10-15: LGTM! Language mapping is accurate
The platform-to-language mapping correctly represents the supported languages for each platform.
17-33: LGTM! Filtering logic with proper error handling
The function correctly filters destinations based on connection modes and includes the requested logging for missing configurations.
What are the changes introduced in this PR?
Add templates for various languages and script to generate constants
What is the related Linear task?
Resolves INT-2900
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
package.jsonfile.Documentation