Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 5 additions & 2 deletions esbuild.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,23 @@
// This may require changes when upgrading to esbuild 0.17+

const { build } = require("esbuild");
const glob = require("glob");

const baseConfig = {
bundle: true,
minify: process.env.NODE_ENV === "production",
sourcemap: process.env.NODE_ENV !== "production",
};

const testFiles = process.env.NODE_ENV !== "production" ? glob.sync("./src/test/**/*.ts") : [];

const extensionConfig = {
...baseConfig,
platform: "node",
mainFields: ["module", "main"],
format: "cjs",
entryPoints: ["./src/extension.ts"],
outfile: "./out/extension.js",
entryPoints: ["./src/extension.ts", ...testFiles],
outdir: "./out",
external: ["vscode"],
};

Expand Down
62 changes: 62 additions & 0 deletions src/astUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as vscode from "vscode";
import {
ArrayASTNode,
ASTNode,
Location,
ObjectASTNode,
Expand Down Expand Up @@ -124,3 +125,64 @@ export function getOptionsFromParamDefinition(
});
return options;
}

export function isJSONDifferentToAST(json: unknown, ast: ASTNode): boolean {
const jsonType =
typeof json === "object"
? Array.isArray(json)
? "array"
: "object"
: typeof json;

// If the property is undefined in the JSON but present in the AST it will return here
if (jsonType !== ast.type) {
return true;
}

if (isArrayASTNode(ast) && Array.isArray(json)) {
if (ast.items.length !== json.length) {
return true;
}

for (let i = 0; i < ast.items.length; i++) {
if (isJSONDifferentToAST(json[i], ast.items[i])) {
return true;
}
}

return false;
}

if (isObjectASTNode(ast) && typeof json === "object" && json !== null) {
if (ast.properties.length !== Object.keys(json).length) {
return true;
}

for (const property of ast.properties) {
if (!property.valueNode) {
return true;
}

if (
isJSONDifferentToAST(
(json as Record<string, unknown>)[property.keyNode.value],
property.valueNode,
)
) {
Comment thread
AlCalzone marked this conversation as resolved.
return true;
}
}

return false;
}

return ast.value !== json;
}

export function isArrayASTNode(node: ASTNode): node is ArrayASTNode {
return node.type === "array";
}

export function isObjectASTNode(node: ASTNode): node is ObjectASTNode {
return node.type === "object";
}
23 changes: 11 additions & 12 deletions src/diagnostics/importOverrideDiagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ASTNode, PropertyASTNode } from "vscode-json-languageservice";
import {
getPropertyNameFromNode,
getPropertyValueFromNode,
isJSONDifferentToAST,
nodeIsPropertyNameOrValue,
rangeFromNode,
} from "../astUtils";
Expand Down Expand Up @@ -62,14 +63,13 @@ export function generateImportOverrideDiagnostics(
const properties = s
.map((s) => config.getNodeFromSymbol(s))
.filter(nodeIsPropertyNameOrValue)
.map(
(n) =>
[
getPropertyNameFromNode(n),
getPropertyValueFromNode(n),
n,
] as const,
)
.map((n) => {
return [
getPropertyNameFromNode(n),
n.parent.valueNode,

Copilot AI Jul 8, 2025

Copy link

Choose a reason for hiding this comment

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

Using n.parent.valueNode returns the AST node rather than the JSON value. Consider using getPropertyValueFromNode(n) to pass the actual JSON value into the diagnostics.

Suggested change
n.parent.valueNode,
getPropertyValueFromNode(n),

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm I missed this change. The change itself is fine, so ignore Copilot's review here. Given that the array now contains the AST node and not its value, the variable in line 82 should be renamed to valueNode.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if the array could be reduced to just the property name , and the ASTNode.

Line 94 could be changed to: value: getPropertyValueFromNode(propNode),.

And line 85 to if (!isJSONDifferentToAST(originalValue, propNode.parent.valueNode))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm that wouldn't change the other type issue with the value possibly being undefined. Let's just stick to renaming then and making sure we pass the actual value to the diagnostic and not the AST node (see other comment).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've changed the variable name as suggested

n,
] as const;
})
.filter(([name]) => name in resolvedImport);
return [resolvedImport, properties] as const;
},
Expand All @@ -79,11 +79,10 @@ export function generateImportOverrideDiagnostics(
if (!block) continue;
const [imp, properties] = block;

for (const [name, value, propNode] of properties) {
for (const [name, valueNode, propNode] of properties) {
const originalValue = imp[name];
const isUnchanged = value === originalValue;

if (isUnchanged) {
if (valueNode && !isJSONDifferentToAST(originalValue, valueNode)) {
ret.push({
type: DiagnosticType.UnnecessaryImportOverride,
range: rangeFromNode(config.original, propNode.parent),
Comment thread
AlCalzone marked this conversation as resolved.
Expand All @@ -92,7 +91,7 @@ export function generateImportOverrideDiagnostics(
ret.push({
type: DiagnosticType.ImportOverride,
range: rangeFromNode(config.original, propNode),
value,
value: getPropertyValueFromNode(propNode),
originalValue,
});
}
Expand Down
Loading