Skip to content

Prevent Complete tsconfig file Overwriting #388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Prevent Complete tsconfig file Overwriting #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7d7e4a0
Add Expo to ignore path removal
Eprince-hub Jun 19, 2024
5ab451c
Refactor install.js to simplify projectType condition
Eprince-hub Jun 19, 2024
c76c612
Ignore path override in tsconfig files
Eprince-hub Jun 19, 2024
fec4e5e
Update tsconfig.json merging logic in install.js
Eprince-hub Jun 20, 2024
eccf7cb
Add check for paths
Eprince-hub Jun 20, 2024
dc6ae3c
Add comment-json package
Eprince-hub Jun 20, 2024
d16c3d5
Remove lockfile diff
Eprince-hub Jun 20, 2024
3f6af5a
Merge branch 'main' of https://github.com/upleveled/eslint-config-upl…
Eprince-hub Jun 20, 2024
8f434da
Update pnpm-lock.yaml
Eprince-hub Jun 20, 2024
3e3ecd5
Update lock file
Eprince-hub Jun 20, 2024
6e76a68
Reorder checks
Eprince-hub Jun 20, 2024
df161e8
Merge branch 'main' into tsconfig-file-overrid
karlhorky Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions bin/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from 'node:fs';
import { dirname, join, resolve } from 'node:path';
import { fileURLToPath } from 'node:url';
import { parse, stringify } from 'comment-json';
import sortPackageJson from 'sort-package-json';

const projectPackageJsonPath = join(process.cwd(), 'package.json');
Expand Down Expand Up @@ -208,6 +209,42 @@ for (const {
// Always overwrite tsconfig.json
if (templateFileName === 'tsconfig.json') {
overwriteExistingFile = true;

const projectTsConfig = parse(readFileSync(filePathInProject, 'utf-8'));
const templateTsConfig = parse(readFileSync(templateFilePath, 'utf-8'));
Comment on lines +213 to +214
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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
Comment on lines +216 to +231
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Simplify validation logic and fix incomplete merge condition.

The validation has two issues:

  1. Verbose validation: The type-checking logic is verbose and could be simplified using the is-plain-obj package as suggested by karlhorky in the PR comments.

  2. Incomplete merge logic: Line 231's condition !templateTsConfig.compilerOptions.paths means 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.

Suggested change
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;
Comment on lines +233 to +234
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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,
};
Comment on lines +236 to +239
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

}

writeFileSync(
filePathInProject,
`${stringify(templateTsConfig, null, 2)}\n`,
);
console.log('✅ Done merging default tsconfig.json');
continue;
}

if (!overwriteExistingFile) {
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
"@next/eslint-plugin-next": "14.2.4",
"@typescript-eslint/eslint-plugin": "8.0.0-alpha.30",
"@typescript-eslint/parser": "8.0.0-alpha.30",
"comment-json": "^4.2.3",
"eslint-config-flat-gitignore": "0.1.5",
"eslint-import-resolver-typescript": "3.6.1",
"eslint-plugin-import-x": "0.5.1",
Expand Down
44 changes: 44 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.