diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index f13eab4ab..0c4216a43 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -44,6 +44,7 @@ This is the **Microsoft Application Insights JavaScript SDK** - a browser-based - Maximum line length: 140 characters ### Naming Conventions +- **Branches**: Use concise, intent-first kebab-case names that describe the change (e.g., `fix-vite8-pure-annotations`). Avoid generic activity names such as `investigate-issue-2764` unless the user explicitly requests them. - **Classes**: PascalCase (e.g., `PageViewManager`, `TelemetryContext`) - **Interfaces**: PascalCase with `I` prefix (e.g., `ITelemetryItem`, `IPageViewTelemetry`) - **Methods/Functions**: camelCase (e.g., `trackPageView`, `sendTelemetry`) diff --git a/AISKU/Tests/Unit/src/AISKUSize.Tests.ts b/AISKU/Tests/Unit/src/AISKUSize.Tests.ts index 88a79eca8..e14bedb23 100644 --- a/AISKU/Tests/Unit/src/AISKUSize.Tests.ts +++ b/AISKU/Tests/Unit/src/AISKUSize.Tests.ts @@ -117,7 +117,7 @@ export class AISKUSizeCheck extends AITestClass { private _checkPureAnnotations(filePath: string, label: string): void { this.testCase({ - name: `Test ${label} canonicalizes PURE annotation spacing`, + name: `Test ${label} contains valid PURE annotations`, test: () => { let request = new Request(filePath, { method: "GET" }); return fetch(request).then((response) => { @@ -131,6 +131,10 @@ export class AISKUSizeCheck extends AITestClass { let nonCanonicalPurePattern = /\(\s+\/\*\s*[#@]__PURE__\s*\*\/|\(\s*\/\*\s*[#@]__PURE__\s*\*\/\s+/g; let matches = text.match(nonCanonicalPurePattern) || []; Assert.equal(0, matches.length, `Found ${matches.length} non-canonical PURE annotations in ${label}`); + + let pureLiteralPattern = /\/\*\s*[#@]__PURE__\s*\*\/\s*(?:null|true|false|["']|[-+]?\d)/g; + matches = text.match(pureLiteralPattern) || []; + Assert.equal(0, matches.length, `Found ${matches.length} PURE annotations on literals in ${label}`); }, (error: Error) => { Assert.ok(false, `${label} PURE annotation check response error: ${error}`); }); diff --git a/package.json b/package.json index eba4898ac..61702f5e6 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,8 @@ "build": "node common/scripts/install-run-rush.js rebuild --verbose", "rebuild": "npm run build && npm run api-docs", "testx": "rush test --verbose", - "test": "node common/scripts/install-run-rush.js test --verbose", + "test": "npm run test:pure-annotations && node common/scripts/install-run-rush.js test --verbose", + "test:pure-annotations": "node tools/pureAnnotations.test.mjs", "mintest": "node common/scripts/install-run-rush.js mintest --verbose", "lint": "node common/scripts/install-run-rush.js lint --verbose", "lint-fix": "npm run ai-restore && grunt lint-fix", @@ -56,6 +57,7 @@ "@rollup/plugin-replace": "^5.0.2", "@typescript-eslint/eslint-plugin": "^7.14.1", "@typescript-eslint/parser": "^7.14.1", + "acorn": "^8.18.0", "archiver": "^5.3.0", "connect": "^3.7.0", "eslint": "^8.56.0", diff --git a/shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts b/shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts index 894f3ab3f..f2b45254f 100644 --- a/shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts +++ b/shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts @@ -92,7 +92,7 @@ export class AppInsightsCoreSizeCheck extends AITestClass { private _checkPureAnnotations(filePath: string, label: string): void { this.testCase({ - name: `Test ${label} canonicalizes PURE annotation spacing`, + name: `Test ${label} contains valid PURE annotations`, test: () => { let request = new Request(filePath, { method: "GET" }); return fetch(request).then((response) => { @@ -106,6 +106,10 @@ export class AppInsightsCoreSizeCheck extends AITestClass { let nonCanonicalPurePattern = /\(\s+\/\*\s*[#@]__PURE__\s*\*\/|\(\s*\/\*\s*[#@]__PURE__\s*\*\/\s+/g; let matches = text.match(nonCanonicalPurePattern) || []; Assert.equal(0, matches.length, `Found ${matches.length} non-canonical PURE annotations in ${label}`); + + let pureLiteralPattern = /\/\*\s*[#@]__PURE__\s*\*\/\s*(?:null|true|false|["']|[-+]?\d)/g; + matches = text.match(pureLiteralPattern) || []; + Assert.equal(0, matches.length, `Found ${matches.length} PURE annotations on literals in ${label}`); }, (error) => { Assert.ok(false, `${label} PURE annotation check response error: ${error}`); }); diff --git a/tools/grunt-tasks/fixPureAnnotations.js b/tools/grunt-tasks/fixPureAnnotations.js index fb035953d..5a124ecc4 100644 --- a/tools/grunt-tasks/fixPureAnnotations.js +++ b/tools/grunt-tasks/fixPureAnnotations.js @@ -10,16 +10,15 @@ * parentheses are required so that older versions of Rollup / Webpack / * Terser still tree-shake the constants, so they must NOT be removed. * However, newer bundlers such as Rolldown (Vite 8) are stricter and reject - * the spaced form, emitting `[INVALID_ANNOTATION]` warnings. This task - * rewrites the spaced form back to the canonical, flush-against-the-paren - * form `(/*#__PURE__*\/...)` which is accepted by every bundler while still - * preserving the tree-shaking behaviour. + * annotations that do not apply to call or new expressions. This task parses + * the generated output, removes invalid annotations, and rewrites valid + * annotations to the canonical form `(/*#__PURE__*\/...)`. * * The `rollup.base.config.js` `fixPureAnnotations()` plugin already performs * this canonicalization for the rollup-bundled `dist/es5` / `browser` CDN * outputs (the package `main` entry). This task closes the gap for the * un-bundled `dist-es5` tsc output (the package `module` entry) which never - * passes through rollup. See issue #2736. + * passes through rollup. See issues #2736, #2763, and #2764. * * Usage in gruntfile: * grunt.loadTasks("./tools/grunt-tasks"); @@ -35,7 +34,7 @@ module.exports = function (grunt) { "use strict"; - grunt.registerMultiTask("fix-pure", "Canonicalize PURE tree-shaking annotations in dist-es5 output", function () { + grunt.registerMultiTask("fix-pure", "Normalize PURE tree-shaking annotations in dist-es5 output", function () { var files = this.filesSrc; var done = this.async(); @@ -51,26 +50,32 @@ module.exports = function (grunt) { var filesChecked = 0; var filesChanged = 0; - files.forEach(function (filepath) { - if (!grunt.file.exists(filepath)) { - return; - } + try { + files.forEach(function (filepath) { + if (!grunt.file.exists(filepath)) { + return; + } - // Skip source map files - only the emitted JavaScript is rewritten. - if (filepath.indexOf(".map") !== -1) { - return; - } + // Skip source map files - only the emitted JavaScript is rewritten. + if (filepath.indexOf(".map") !== -1) { + return; + } - filesChecked++; + filesChecked++; - var content = grunt.file.read(filepath); - var normalized = canonicalizePureAnnotations(content); + var content = grunt.file.read(filepath); + var normalized = canonicalizePureAnnotations(content); - if (normalized !== content) { - grunt.file.write(filepath, normalized); - filesChanged++; - } - }); + if (normalized !== content) { + grunt.file.write(filepath, normalized); + filesChanged++; + } + }); + } catch (err) { + grunt.log.error("Failed to normalize PURE annotations: " + err); + done(false); + return; + } grunt.log.ok("Canonicalized PURE annotations: checked " + filesChecked + " file(s), updated " + filesChanged + " file(s)."); done(); diff --git a/tools/pureAnnotations.mjs b/tools/pureAnnotations.mjs index ed2562aab..c701f0e8a 100644 --- a/tools/pureAnnotations.mjs +++ b/tools/pureAnnotations.mjs @@ -1,37 +1,132 @@ /** - * pureAnnotations.mjs - Shared helpers for canonicalizing `/*#__PURE__*\/` - * (and `/*#@__PURE__*\/`) tree-shaking annotations. + * pureAnnotations.mjs - Shared helpers for normalizing `/*#__PURE__*\/` + * (and `/*@__PURE__*\/`) tree-shaking annotations. * - * TypeScript emits parenthesized PURE annotations with whitespace after the - * opening parenthesis, e.g. `( /*#__PURE__*\/"http.")`. The wrapping - * parentheses are required so that older versions of Rollup / Webpack / Terser - * still tree-shake the constants, so they must NOT be removed. However, newer - * bundlers such as Rolldown (Vite 8) are stricter and reject the spaced form, - * emitting `[INVALID_ANNOTATION]` warnings. Canonicalizing to the flush form - * `(/*#__PURE__*\/...)` is accepted by every bundler while preserving the - * tree-shaking behaviour. + * PURE annotations are only meaningful on call and new expressions. The + * source retains legacy parenthesized annotations, while generated output is + * parsed so invalid annotations can be removed without touching annotation + * text inside strings or unrelated comments. * * This single source of truth is shared by: * - rollup.base.config.js `fixPureAnnotations()` (rollup-bundled dist/es5), * which imports it directly (rollup inlines it when bundling the config). * - tools/grunt-tasks/fixPureAnnotations.js `fix-pure` (tsc dist-es5), which * loads it via dynamic import() from the CommonJS grunt task. - * - * It is authored as an ES module (.mjs) so the rollup config bundler (which - * does not run the commonjs plugin) can consume the named exports. */ -// Matches an opening parenthesis followed by a (possibly whitespace padded) -// PURE / @__PURE__ annotation, capturing the leading marker char (# or @). -export var PURE_COMMENT_CANONICALIZE = /\(\s*\/\*\s*([#@])__PURE__\s*\*\/\s*/g; +import { parse } from "acorn"; + +var PURE_COMMENT = /^\s*([#@])__PURE__\s*$/; /** - * Rewrites any spaced PURE annotation forms in the supplied code to the - * canonical flush-against-the-paren form. Returns the (possibly unchanged) - * code string. + * Removes PURE annotations that do not apply to a call or new expression and + * canonicalizes valid annotations. Returns the (possibly unchanged) code. * @param {string} code * @returns {string} */ export function canonicalizePureAnnotations(code) { - return code.replace(PURE_COMMENT_CANONICALIZE, "(/*$1__PURE__*/"); + if (code.indexOf("__PURE__") === -1) { + return code; + } + + var comments = []; + var tokens = []; + var validExpressionStarts = {}; + + var ast = parse(code, { + allowHashBang: true, + ecmaVersion: "latest", + onComment: comments, + onToken: tokens, + sourceType: "module" + }); + + _collectValidExpressionStarts(ast, validExpressionStarts); + + var edits = []; + comments.forEach(function (comment) { + if (comment.type !== "Block") { + return; + } + + var pureMatch = PURE_COMMENT.exec(comment.value); + if (!pureMatch) { + return; + } + + var tokenIndex = _findNextToken(tokens, comment.end); + var immediateToken = tokens[tokenIndex]; + var isValid = !!(immediateToken && validExpressionStarts[immediateToken.start]); + var start = comment.start; + var previous = start - 1; + while (previous >= 0 && /\s/.test(code.charAt(previous))) { + previous--; + } + + if (code.charAt(previous) === "(") { + start = previous + 1; + } + + var end = comment.end; + if (immediateToken && /^\s*$/.test(code.substring(end, immediateToken.start))) { + end = immediateToken.start; + } + + edits.push({ + end: end, + replacement: isValid ? "/*" + pureMatch[1] + "__PURE__*/" : "", + start: start + }); + }); + + for (var lp = edits.length - 1; lp >= 0; lp--) { + var edit = edits[lp]; + code = code.substring(0, edit.start) + edit.replacement + code.substring(edit.end); + } + + return code; +} + +function _collectValidExpressionStarts(node, validExpressionStarts) { + var pending = [node]; + + while (pending.length > 0) { + var current = pending.pop(); + if (!current || typeof current !== "object") { + continue; + } + + if (current.type === "CallExpression" || current.type === "NewExpression") { + validExpressionStarts[current.start] = true; + } + + Object.keys(current).forEach(function (key) { + var value = current[key]; + if (Array.isArray(value)) { + value.forEach(function (child) { + if (child && typeof child === "object") { + pending.push(child); + } + }); + } else if (value && typeof value === "object" && typeof value.type === "string") { + pending.push(value); + } + }); + } +} + +function _findNextToken(tokens, position) { + var low = 0; + var high = tokens.length; + + while (low < high) { + var middle = (low + high) >> 1; + if (tokens[middle].start < position) { + low = middle + 1; + } else { + high = middle; + } + } + + return low; } diff --git a/tools/pureAnnotations.test.mjs b/tools/pureAnnotations.test.mjs new file mode 100644 index 000000000..429187888 --- /dev/null +++ b/tools/pureAnnotations.test.mjs @@ -0,0 +1,103 @@ +import assert from "node:assert/strict"; +import { canonicalizePureAnnotations } from "./pureAnnotations.mjs"; + +var invalidExpressions = [ + "null", + "true", + "false", + "\"string\"", + "'string'", + "42", + "42n", + "0xff", + "0b1010", + "0o755", + "1.5e2", + "/regex/", + "`template`", + "[]", + "{}", + "function () {}", + "() => 1", + "class {}", + "identifier", + "!factory()", + "left + right", + "condition ? left : right", + "(factory(), identifier)", + "(factory())", + "((factory)())" +]; + +invalidExpressions.forEach(function (expression) { + var input = "var value = ( /* @__PURE__ */ " + expression + ");"; + var expected = "var value = (" + expression + ");"; + assert.equal(canonicalizePureAnnotations(input), expected, "removes PURE from " + expression); +}); + +var validCases = [ + { + expected: "var value = (/*#__PURE__*/factory());", + input: "var value = ( /*#__PURE__*/ factory());" + }, + { + expected: "var value = (/*@__PURE__*/new Factory());", + input: "var value = ( /* @__PURE__ */ new Factory());" + }, + { + expected: "var value = /*#__PURE__*/namespace.factory();", + input: "var value = /* #__PURE__ */ namespace.factory();" + }, + { + expected: "var value = /*#__PURE__*/(function () {})();", + input: "var value = /* #__PURE__ */ (function () {})();" + }, + { + expected: "var value = /*#__PURE__*/(0, factory)();", + input: "var value = /* #__PURE__ */ (0, factory)();" + }, + { + expected: "var value = (/*#__PURE__*/factory().value);", + input: "var value = ( /* #__PURE__ */ factory().value);" + }, + { + expected: "var value = (/*@__PURE__*/factory?.());", + input: "var value = ( /* @__PURE__ */ factory?.());" + }, + { + expected: "var value = (/*#__PURE__*/new Factory().value);", + input: "var value = ( /* #__PURE__ */ new Factory().value);" + }, + { + expected: "export var value = (/*#__PURE__*/factory());", + input: "export var value = ( /* #__PURE__ */ \nfactory());" + } +]; + +validCases.forEach(function (testCase) { + assert.equal(canonicalizePureAnnotations(testCase.input), testCase.expected, "preserves PURE on call/new"); +}); + +var untouchedCases = [ + "var value = \"(/*#__PURE__*/ literal)\";", + "var value = '/*@__PURE__*/ null';", + "var value = `/*#__PURE__*/ false`;", + "// /*#__PURE__*/ null\nvar value = null;", + "/* an ordinary comment */ var value = null;", + "var value = 42;" +]; + +untouchedCases.forEach(function (code) { + assert.equal(canonicalizePureAnnotations(code), code, "leaves non-annotation text unchanged"); +}); + +assert.throws(function () { + canonicalizePureAnnotations("var = /* #__PURE__ */ invalid;"); +}, SyntaxError, "surfaces invalid generated JavaScript"); + +validCases.forEach(function (testCase) { + var normalized = canonicalizePureAnnotations(testCase.input); + assert.equal(canonicalizePureAnnotations(normalized), normalized, "normalization is idempotent"); +}); + +console.log("PURE annotation normalization tests passed");