Prevent Complete tsconfig file Overwriting #388
Conversation
…eveled into tsconfig-file-overrid
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
@Eprince-hub probably this will be simpler with |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughEnhances bin/install.js to parse and merge tsconfig.json using comment-json, preserving and merging compilerOptions.paths from project and template, with template paths taking precedence. Writes the merged result and skips overwrite logic for that file. Adds comment-json dependency in package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as bin/install.js
participant FS as Filesystem
participant CJ as comment-json
Dev->>CLI: Run install
loop For each template file
CLI->>FS: Check target file
alt File is tsconfig.json
CLI->>FS: Read project tsconfig.json
CLI->>FS: Read template tsconfig.json
CLI->>CJ: parse(project), parse(template)
CJ-->>CLI: projectObj, templateObj
CLI->>CLI: Validate objects and compilerOptions
CLI->>CLI: Merge compilerOptions.paths (template precedence)
CLI->>CJ: stringify(mergedTemplate)
CJ-->>CLI: mergedText
CLI->>FS: Write merged tsconfig.json
CLI->>Dev: Log merge success
note over CLI: Skip standard overwrite for this file
else Other files
CLI->>FS: Proceed with existing copy/overwrite logic
end
end
CLI-->>Dev: Done
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a merge strategy for tsconfig.json files to prevent complete overwriting during UpLeveled ESLint config installation, preserving existing configurations like paths and other settings to resolve TypeScript errors in React Native Expo projects.
- Added
comment-jsondependency to handle JSON files with comments during the merge process
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
bin/install.js (1)
242-247: LGTM!The file write operation correctly uses
stringify()from comment-json to preserve comments in the output. Thecontinuestatement appropriately skips the remaining overwrite logic, and the success message provides helpful feedback.Optional nitpick: Line 244's template literal could be simplified to
stringify(templateTsConfig, null, 2) + '\n'for consistency with line 67, though the current approach works fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
bin/install.js(2 hunks)package.json(1 hunks)
🔇 Additional comments (2)
package.json (1)
44-44: LGTM!The comment-json dependency is correctly placed in
dependencies(notdevDependencies) since it's required by the CLI tool at runtime. The version range^4.2.3is appropriate and will accept the latest stable version 4.2.5.Based on learnings.
bin/install.js (1)
15-15: LGTM!The import statement correctly follows the comment-json API as documented.
| const projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8')); | ||
| const templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8')); |
There was a problem hiding this comment.
Add error handling for JSON parsing.
The parse() calls can throw exceptions if the JSON files are malformed or contain syntax errors. This could crash the installation process without helpful error messages.
Apply this diff to add error handling:
- const projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8'));
- const templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8'));
+ let projectTsConfig, templateTsConfig;
+ try {
+ projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8'));
+ templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8'));
+ } catch (error) {
+ console.error(`Error parsing tsconfig.json: ${error.message}`);
+ console.log('Skipping tsconfig.json merge, using template as-is');
+ cpSync(templateFilePath, filePathInProject);
+ continue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8')); | |
| const templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8')); | |
| let projectTsConfig, templateTsConfig; | |
| try { | |
| projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8')); | |
| templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8')); | |
| } catch (error) { | |
| console.error(`Error parsing tsconfig.json: ${error.message}`); | |
| console.log('Skipping tsconfig.json merge, using template as-is'); | |
| cpSync(templateFilePath, filePathInProject); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In bin/install.js around lines 213 to 214, the parse(readFileSync(...)) calls
can throw on malformed JSON; wrap each read+parse in a try/catch that catches
JSON/syntax errors, log a clear error indicating which file (filePathInProject
or templateFilePath) failed to parse and include the error.message, then either
rethrow or exit the process with a non-zero code so the installer fails fast
with a helpful message; ensure the rest of the flow only continues if both
parses succeeded.
| if ( | ||
| projectTsConfig && | ||
| templateTsConfig && | ||
| typeof projectTsConfig === 'object' && | ||
| !Array.isArray(projectTsConfig) && | ||
| projectTsConfig.compilerOptions && | ||
| typeof projectTsConfig.compilerOptions === 'object' && | ||
| !Array.isArray(projectTsConfig.compilerOptions) && | ||
| projectTsConfig.compilerOptions.paths && | ||
| typeof projectTsConfig.compilerOptions.paths === 'object' && | ||
| typeof templateTsConfig === 'object' && | ||
| !Array.isArray(templateTsConfig) && | ||
| templateTsConfig.compilerOptions && | ||
| typeof templateTsConfig.compilerOptions === 'object' && | ||
| !Array.isArray(templateTsConfig.compilerOptions) && | ||
| !templateTsConfig.compilerOptions.paths |
There was a problem hiding this comment.
Simplify validation logic and fix incomplete merge condition.
The validation has two issues:
-
Verbose validation: The type-checking logic is verbose and could be simplified using the
is-plain-objpackage as suggested by karlhorky in the PR comments. -
Incomplete merge logic: Line 231's condition
!templateTsConfig.compilerOptions.pathsmeans paths are only merged when the template has NO paths. If the template HAS paths, the project's paths are completely lost, contradicting the PR objective of "combining existing project tsconfig configuration with new template configuration."
Fix the merge logic to handle all cases:
+import isPlainObj from 'is-plain-obj';
+
// ... in the tsconfig.json handling block ...
if (
- projectTsConfig &&
- templateTsConfig &&
- typeof projectTsConfig === 'object' &&
- !Array.isArray(projectTsConfig) &&
- projectTsConfig.compilerOptions &&
- typeof projectTsConfig.compilerOptions === 'object' &&
- !Array.isArray(projectTsConfig.compilerOptions) &&
- projectTsConfig.compilerOptions.paths &&
- typeof projectTsConfig.compilerOptions.paths === 'object' &&
- typeof templateTsConfig === 'object' &&
- !Array.isArray(templateTsConfig) &&
- templateTsConfig.compilerOptions &&
- typeof templateTsConfig.compilerOptions === 'object' &&
- !Array.isArray(templateTsConfig.compilerOptions) &&
- !templateTsConfig.compilerOptions.paths
+ isPlainObj(projectTsConfig) &&
+ isPlainObj(projectTsConfig.compilerOptions) &&
+ isPlainObj(projectTsConfig.compilerOptions.paths) &&
+ isPlainObj(templateTsConfig) &&
+ isPlainObj(templateTsConfig.compilerOptions)
) {
- templateTsConfig.compilerOptions.paths =
- projectTsConfig.compilerOptions.paths;
-
- templateTsConfig.compilerOptions.paths = {
- ...projectTsConfig.compilerOptions.paths,
- ...templateTsConfig.compilerOptions.paths,
- };
+ const { assign } = await import('comment-json');
+
+ if (isPlainObj(templateTsConfig.compilerOptions.paths)) {
+ // Template has paths - merge with project paths (template takes precedence)
+ templateTsConfig.compilerOptions.paths = assign(
+ {},
+ projectTsConfig.compilerOptions.paths,
+ templateTsConfig.compilerOptions.paths
+ );
+ } else {
+ // Template has no paths - preserve project paths
+ templateTsConfig.compilerOptions.paths = assign(
+ {},
+ projectTsConfig.compilerOptions.paths
+ );
+ }
}Note: You'll also need to add is-plain-obj to dependencies in package.json.
As per coding guidelines (referenced by karlhorky).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| projectTsConfig && | |
| templateTsConfig && | |
| typeof projectTsConfig === 'object' && | |
| !Array.isArray(projectTsConfig) && | |
| projectTsConfig.compilerOptions && | |
| typeof projectTsConfig.compilerOptions === 'object' && | |
| !Array.isArray(projectTsConfig.compilerOptions) && | |
| projectTsConfig.compilerOptions.paths && | |
| typeof projectTsConfig.compilerOptions.paths === 'object' && | |
| typeof templateTsConfig === 'object' && | |
| !Array.isArray(templateTsConfig) && | |
| templateTsConfig.compilerOptions && | |
| typeof templateTsConfig.compilerOptions === 'object' && | |
| !Array.isArray(templateTsConfig.compilerOptions) && | |
| !templateTsConfig.compilerOptions.paths | |
| // At the top of bin/install.js | |
| import isPlainObj from 'is-plain-obj'; | |
| // ... in the tsconfig.json handling block ... | |
| if ( | |
| isPlainObj(projectTsConfig) && | |
| isPlainObj(projectTsConfig.compilerOptions) && | |
| isPlainObj(projectTsConfig.compilerOptions.paths) && | |
| isPlainObj(templateTsConfig) && | |
| isPlainObj(templateTsConfig.compilerOptions) | |
| ) { | |
| const { assign } = await import('comment-json'); | |
| if (isPlainObj(templateTsConfig.compilerOptions.paths)) { | |
| // Template has paths — merge with project paths (template takes precedence) | |
| templateTsConfig.compilerOptions.paths = assign( | |
| {}, | |
| projectTsConfig.compilerOptions.paths, | |
| templateTsConfig.compilerOptions.paths | |
| ); | |
| } else { | |
| // Template has no paths — preserve project paths | |
| templateTsConfig.compilerOptions.paths = assign( | |
| {}, | |
| projectTsConfig.compilerOptions.paths | |
| ); | |
| } | |
| } |
| templateTsConfig.compilerOptions.paths = | ||
| projectTsConfig.compilerOptions.paths; |
There was a problem hiding this comment.
Remove redundant assignment.
Lines 233-234 assign the project paths to template paths, but this assignment is immediately overwritten by lines 236-239. Since the condition on line 231 ensures the template has no paths, this assignment serves no purpose.
Apply this diff to remove the redundant code:
- templateTsConfig.compilerOptions.paths =
- projectTsConfig.compilerOptions.paths;
-
templateTsConfig.compilerOptions.paths = {
...projectTsConfig.compilerOptions.paths,
...templateTsConfig.compilerOptions.paths,
};Note: This comment applies to the current implementation. If you apply the refactor suggested for lines 216-231, this entire section will be replaced with the corrected merge logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| templateTsConfig.compilerOptions.paths = | |
| projectTsConfig.compilerOptions.paths; | |
| templateTsConfig.compilerOptions.paths = { | |
| ...projectTsConfig.compilerOptions.paths, | |
| ...templateTsConfig.compilerOptions.paths, | |
| }; |
🤖 Prompt for AI Agents
In bin/install.js around lines 233 to 234, remove the redundant assignment
"templateTsConfig.compilerOptions.paths =
projectTsConfig.compilerOptions.paths;" because it gets overwritten by the
subsequent merge (lines 236-239); delete these two lines so the template paths
are not needlessly set before they are replaced.
| templateTsConfig.compilerOptions.paths = { | ||
| ...projectTsConfig.compilerOptions.paths, | ||
| ...templateTsConfig.compilerOptions.paths, | ||
| }; |
There was a problem hiding this comment.
Object spread loses comment metadata.
Using the object spread operator ({...obj}) does not preserve comment metadata attached to the parsed objects. This defeats the purpose of using comment-json, as comments from the original files will be lost.
Per the comment-json documentation, you must use assign() to preserve comment metadata when copying objects:
+ const { assign } = await import('comment-json');
+
templateTsConfig.compilerOptions.paths =
- {
- ...projectTsConfig.compilerOptions.paths,
- ...templateTsConfig.compilerOptions.paths,
- };
+ assign(
+ {},
+ projectTsConfig.compilerOptions.paths,
+ templateTsConfig.compilerOptions.paths
+ );Note: This comment applies to the current implementation. If you apply the refactor suggested for lines 216-231, this section will be replaced with the corrected merge logic that already uses assign().
Based on learnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| templateTsConfig.compilerOptions.paths = { | |
| ...projectTsConfig.compilerOptions.paths, | |
| ...templateTsConfig.compilerOptions.paths, | |
| }; | |
| const { assign } = await import('comment-json'); | |
| templateTsConfig.compilerOptions.paths = assign( | |
| {}, | |
| projectTsConfig.compilerOptions.paths, | |
| templateTsConfig.compilerOptions.paths | |
| ); |
🤖 Prompt for AI Agents
In bin/install.js around lines 236 to 239, the object spread used to merge
compilerOptions.paths will strip comment-json metadata; replace the spread merge
with comment-json.assign(templateTsConfig.compilerOptions.paths,
projectTsConfig.compilerOptions.paths, templateTsConfig.compilerOptions.paths)
(or equivalent assign call that merges while preserving metadata) and ensure
comment-json.assign is imported/available; maintain the same merge order so
project paths override template paths where intended and remove the spread usage
so comments are kept.
2 participants
Implement a merge strategy for
tsconfig.jsonfiles to combine existing configurations with new template configurations during the UpLeveled ESLint config installation, to preserve paths and other settings and solve the TypeScript error in react native expo projectsSummary by CodeRabbit
New Features
Chores