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
30 changes: 23 additions & 7 deletions packages/ts-transformers/src/core/ct-helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,8 @@ export class CTHelpers {
export function transformCtDirective(
source: string,
): string {
// Throw when this symbol name is already in use
// for some reason.
if (source.indexOf(CT_HELPERS_IDENTIFIER) !== -1) {
throw new Error(
`Source cannot contain reserved '${CT_HELPERS_IDENTIFIER}' symbol.`,
);
}
checkCTHelperVar(source);

const lines = source.split("\n");
if (!lines[0] || !isCTSEnabled(lines[0])) {
return source;
Expand All @@ -101,6 +96,27 @@ function isCTSEnabled(line: string) {
return /^\/\/\/\s*<cts-enable\s*\/>/m.test(line);
}

// Throws if `__ctHelpers` was found as an Identifier
// in the source code.
function checkCTHelperVar(source: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, it's very cool that this is actually faster than the string check - i for sure would not have guessed that!

const sourceFile = ts.createSourceFile(
"source.tsx",
source,
ts.ScriptTarget.ES2023,
);
const visitor = (node: ts.Node): ts.Node => {
if (ts.isIdentifier(node)) {
if (node.text === CT_HELPERS_IDENTIFIER) {
throw new Error(
`Source cannot contain reserved '${CT_HELPERS_IDENTIFIER}' symbol.`,
);
}
}
return ts.visitEachChild(node, visitor, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

claude suggests that you could consider using ts.forEachChild(node, visitor) just to avoid having the potentially confusing undefined third parameter for the TransformationContext. i think it's probably good to have it this way because we use visitEachChild elsewhere and it keeps us consistent, but, just mentioning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think forEachChild works as well, changing the return value to ts.Node | undefined, though the iteration logic seems different, but looking more into it (tests pass with it however)

};
ts.visitNode(sourceFile, visitor);
}

function getCTHelpersIdentifier(
statement: ts.Statement,
): ts.Identifier | undefined {
Expand Down
36 changes: 35 additions & 1 deletion packages/ts-transformers/test/transform.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, it } from "@std/testing/bdd";
import { assert } from "@std/assert";
import { assert, assertRejects } from "@std/assert";
import { transformFiles } from "./utils.ts";

const fixture = `
Expand Down Expand Up @@ -34,3 +34,37 @@ describe("CommonToolsTransformerPipeline", () => {
);
});
});

describe("CTHelpers handling", () => {
it("Throws if __ctHelpers variable is used in source", async () => {
const statements = [
"function __ctHelpers() {}",
"function foo(): number { var __ctHelpers = 5; return __ctHelpers; }",
"var __ctHelpers: number = 5;",
"declare global { var __ctHelpers: any; }\nglobalThis.__ctHelpers = 5;",
];

for (const statement of statements) {
await assertRejects(() =>
transformFiles({
"/main.ts": fixture + `\n${statement}`,
})
);
}
});

it("Allows '__ctHelpers' in comments and in other forms", async () => {
const statements = [
"var x = 5; // __ctHelpers",
"// __ctHelpers",
"/* __ctHelpers */",
"var __ctHelpers123: number = 5;",
"declare global {\nvar __ctHelpers1: any;\n}\nglobalThis.__ctHelpers1 = 5;",
];
for (const statement of statements) {
await transformFiles({
"/main.ts": fixture + `\n${statement}`,
});
}
});
});