fix: remove invalid PURE literal annotations and add bundle validation tests#2737
Merged
hectorhdzg merged 22 commits intoJun 18, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses build/bundler compatibility by removing invalid /*#__PURE__*/ annotations placed on non-call/non-new expressions (e.g., string literals / null) and adds automated bundle validations to prevent regressions.
Changes:
- Remove invalid
/*#__PURE__*/annotations applied to literals /nullin source constants. - Switch
ExtVersionback to the build-time#extVersion#placeholder. - Add unit “size” tests that scan built dist bundles for invalid PURE annotation placement (Rolldown-focused validation).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| shared/AppInsightsCore/Tests/Unit/src/ai/AppInsightsCoreSize.Tests.ts | Adds a bundle scan test to detect invalid PURE annotation placement in the AppInsightsCore dist output. |
| shared/AppInsightsCore/src/ext/extUtils.ts | Removes PURE-on-null and restores ExtVersion to the #extVersion# build placeholder. |
| AISKU/Tests/Unit/src/AISKUSize.Tests.ts | Adds a bundle scan test to detect invalid PURE annotation placement in the AISKU dist output. |
| AISKU/src/internal/trace/spanUtils.ts | Removes PURE annotations incorrectly applied to string literal constants. |
Keep legacy parenthesized PURE markers in source and canonicalize emitted bundle annotations as a final rollup plugin pass. Update AISKU and AppInsightsCore size tests to validate canonical PURE spacing in dist output.
hectorhdzg
force-pushed
the
fix/rolldown-pure-annotation-validation
branch
from
37d850c to
ad95fbd
Compare
hectorhdzg
force-pushed
the
fix/rolldown-pure-annotation-validation
branch
from
56aa4f6 to
2075b26
Compare
Set fail-fast: false so a failure on one Node.js version no longer cancels the other matrix jobs, allowing us to see whether failures reproduce across all versions or only some. Also revert channels/offline-channel-js offlinebatchhandler.tests.ts back to main's version, removing the unstable refactor that was causing the 'test1 with unload events' IndexedDB tests to time out in CI.
The branch had switched qunit to a system-installed Chrome (gruntfile _getPuppeteerExecutablePath) combined with PUPPETEER_SKIP_DOWNLOAD=true. On the CI Linux runners the system Chrome is version-mismatched with the pinned puppeteer API, causing IndexedDB operations in headless 'new' mode to hang. Every async IndexedDB test (e.g. 'send Next Batch with IndexedDB provider test1 with unload events') then timed out at 30000ms, producing 9 failures and a ~195s offline-channel run. main is green because it uses puppeteer's bundled, version-matched Chrome. Revert gruntfile.js to main's puppeteer config and drop PUPPETEER_SKIP_DOWNLOAD so the offline-channel tests run against the same Chrome as main. Keep fail-fast: false for matrix diagnostics.
Both prior approaches failed in CI:
- Bundled auto-download (main): npm install crashes because the combined
chrome + chrome-headless-shell postinstall download is flaky on runners
('folder exists but executable is missing' = partial extraction).
- Skip download + system Chrome: IndexedDB hangs in headless 'new' mode
(version-mismatched Chrome) -> offline-channel tests time out at 30s.
Combine the safe parts of both: keep PUPPETEER_SKIP_DOWNLOAD=true so the
flaky postinstall never hard-fails npm install, then install the
version-matched Chrome explicitly in a dedicated step that clears the cache
and retries up to 3 times to recover from partial downloads. The reverted
gruntfile no longer overrides executablePath, so puppeteer auto-discovers
this cached, version-matched Chrome (which does not hang on IndexedDB).
Validated ci.yml YAML and the retry-loop bash syntax locally.
The shims build failed on CI Node 24 because tools/shims/Tests/UnitTests.html had a branch-only edit (modules.run() require errback changed to 'false') that breaks grunt shims (build:esm runs the unit test page during build). main's version is green on Node 24. Revert all remaining branch-only build/tooling churn back to main, since main is green on Node 24 without any of it: - tools/shims/Tests/UnitTests.html (build failure cause) - tools/updateDistEsm/updateDistEsm.js (dynamicproto path; main uses short path) - common/Tests/Framework/rollup.config.js (tslib remap arg) - tools/grunt-tasks/minifyNames.js (retry sleep; no-op on Linux CI) The branch now contains only the real PR (PURE annotation removal/normalization in rollup.base.config.js + extUtils.ts, new bundle validation size tests), the coupled size-threshold bumps, and the working puppeteer chrome CI fix.
Root cause of the shims build failure: grunt shims (build:esm) runs a
qunit BROWSER test as part of the build (registerTask('shims') concats
tsTestActions). My branch's ci.yml used PUPPETEER_SKIP_DOWNLOAD + a manual
'npx puppeteer browsers install chrome chrome-headless-shell', which installs
the LATEST Chrome rather than the version the repo's pinned puppeteer expects.
When grunt qunit launched during the shims build it could not find puppeteer's
expected Chrome build and failed immediately (~6s) -> build:esm exit 1 -> shims
build failed.
main's ci.yml is green because its plain 'npm install' lets puppeteer's
postinstall download the exact version-matched Chrome that grunt qunit uses.
Revert ci.yml entirely to main's known-green baseline. The branch now contains
ONLY the real PR: PURE annotation removal/normalization (rollup.base.config.js,
extUtils.ts) and the coupled bundle-size validation tests/threshold bumps.
Root cause of BOTH CI failures, now confirmed: 1) Download crash on 'npm install grunt-cli': puppeteer's postinstall download of chrome-headless-shell is flaky on the runners (the runner image carries a partial/mismatched puppeteer cache -> 'folder exists but executable is missing' -> install hard-fails). 2) Shims build failure (~6s): my earlier fix ran 'npx puppeteer browsers install chrome chrome-headless-shell' as ONE command. The puppeteer CLI 'browsers install [browser]' takes a SINGLE browser positional, so only 'chrome' was installed and 'chrome-headless-shell' was silently ignored. The grunt qunit task launches puppeteer with headless:'new' which resolves to chrome-headless-shell in puppeteer 24; with SKIP_DOWNLOAD blocking auto-download and chrome-headless-shell missing, the browser tests that run DURING the build (grunt shims concats qunit) failed to launch. Fix: keep PUPPETEER_SKIP_DOWNLOAD to avoid the flaky postinstall, then run 'npx puppeteer browsers install' (no arg = install ALL pinned browsers, both chrome and chrome-headless-shell, at the exact pinned versions) with a clean-cache + retry loop to recover from partial/corrupt downloads. Branch still contains only the real PR plus this single ci.yml fix.
…packages)
THE actual root cause of the recurring shims build failure:
registerTask('shims') was the ONLY package whose BUILD task concatenated
tsTestActions, which appends 'qunit:shims' - i.e. it launched a headless
browser DURING the build. Every other package keeps build (tsBuildActions)
and test (tsTestActions) separate. That is why only @microsoft/applicationinsights-shims
failed during 'rush rebuild' (npm run build), at the ~8s browser-launch step,
identically on Node 18 and Node 24 - it had nothing to do with the source.
shims already has a separate 'shimstest' task (tsTestActions) used by
'npm run test' / rush test, so deferring the qunit there loses no coverage.
Change: registerTask('shims', tsBuildActions('shims')) - build only.
Verified locally:
- npm run build:esm (grunt shims): completes with NO browser launch, exit 0.
- npm run test (grunt shimstest): runs all 8 qunit tests, 0 failed, exit 0.
This removes the browser dependency from the build entirely, so the shims
build can no longer fail on runner Chrome flakiness.
…er SKIP_DOWNLOAD) Confirmed locally (puppeteer 24.43, pinned Chrome 148.0.7778.97): - 'npx puppeteer browsers install' (no arg) WITH PUPPETEER_SKIP_DOWNLOAD=true => exits 0, prints nothing, installs NOTHING (no-op). - 'npx puppeteer browsers install chrome' (single arg) WITH the same env => installs chrome 148.0.7778.97 (exit 0). - 'npx puppeteer browsers install chrome-headless-shell' likewise installs. The job-level PUPPETEER_SKIP_DOWNLOAD=true (needed to gate the flaky postinstall download) ALSO applied to the install step, so the no-arg form silently installed nothing. The retry loop 'succeeded' on attempt 1 having installed nothing, so Chrome was never in ~/.cache/puppeteer and 'rush test' failed: 'Fatal error: Could not find Chrome (ver. 148.0.7778.97)'. Fix: install each browser explicitly (chrome AND chrome-headless-shell), which works regardless of PUPPETEER_SKIP_DOWNLOAD, and log 'puppeteer browsers list' for visibility. Note: the prior shims build-only fix worked - this run got past the build into 'rush test', which is where this Chrome-not-found surfaced.
…nnotation-validation # Conflicts: # .github/workflows/ci.yml # AISKU/Tests/Unit/src/AISKUSize.Tests.ts # AISKULight/Tests/Unit/src/AISKULightSize.Tests.ts # channels/1ds-post-js/test/Unit/src/FileSizeCheckTest.ts
rads-1996
approved these changes
Jun 18, 2026
hectorhdzg
added a commit
that referenced
this pull request
Jun 18, 2026
…ies (#2733) * fix: Migrate from npm to pnpm and resolve all dependency vulnerabilities - Migrate Rush package manager from npm 9.9.4 to pnpm 9.15.9 to eliminate bundled tar 6.2.x CVEs - Upgrade Rush from 5.172.1 to 5.175.1 for pnpm support - Add pnpm-config.json with global overrides for minimatch, tar, glob, lodash, and postcss - Remove unused autoprefixer dependency (source of postcss CVEs) - Add 177 missing phantom dependencies across 27 packages for pnpm strict module isolation - Fix tsconfig lib settings across 26+ packages for @nevware21/ts-utils ES2015+ type compatibility - Fix eventemitter2 export structure change in tools/grunt-tasks/qunit.js - Fix cross-package script resolution in tools/subResourceIntegrity and tools/release-tools - Add Node 24 ESLint 8 compatibility guard in gruntfile.js - Delete npm-shrinkwrap.json, add pnpm-lock.yaml Resolves: 6 tar CVEs (critical/high), 2 postcss CVEs (moderate) — 0 vulnerabilities remaining * fix: use explicit dynamicproto rollup plugin path * fix: update @nevware21/ts-utils version constraint to 0.14.0+ for consistency * fix: apply build/CI improvements from PR 2737 - Add PUPPETEER_SKIP_DOWNLOAD env var in CI - Add puppeteer executable path detection for Linux/CI environments - Fix cross-platform sleep in minifyNames using Atomics.wait - Fix updateDistEsm dynamicproto import path - Fix test module loading in shims tests * fix: resolve grunt task references for pnpm workspace isolation These packages need explicit paths to the root gruntfile since pnpm uses strict dependency isolation. Direct grunt task calls fail without specifying the gruntfile location. * fix(deps): add pnpm overrides to resolve transitive vulnerabilities Regenerated pnpm-lock.yaml after merging upstream/main and added global overrides for 4 advisories surfaced by 'pnpm audit': - ws >=8.21.0 (GHSA-96hv-2xvq-fx4p, high) via puppeteer - form-data >=2.5.6 <3.0.0 (GHSA-hmw2-7cc7-3qxx, high) - js-yaml >=4.2.0 (GHSA-h67p-54hq-rp68, moderate) via eslint/grunt - markdown-it >=14.2.0 (GHSA-6v5v-wf23-fmfq, moderate) via typedoc form-data pinned to the 2.x line (2.5.6 exists) to stay API-compatible with its single 2.x consumer; js-yaml already resolved uniformly to 4.x so 4.2.0 is safe. 'pnpm audit' (prod and full) now reports 0 vulnerabilities. * fix(build): repair malformed perf qunit puppeteer config in gruntfile A prior refactor extracted the inline perf puppeteer options into the 'perfPuppeteerOptions' variable and replaced the inline object with 'puppeteer: perfPuppeteerOptions', but left behind the old object's trailing ']' and an extra '}'. This produced 'SyntaxError: Unexpected token ]' when grunt loaded the gruntfile, which broke the very first build operation (@microsoft/ai-test-framework -> 'grunt tst-framework') and cascaded to block all 27 downstream package builds in CI. Removed the stray ']' and extra '}'. Verified: gruntfile now parses (node require) and 'rush build' completes successfully (exit 0). * ci: temporarily run Node.js CI on push for the pnpm migration branch Adds fix/migrate-pnpm-resolve-dependency-vulnerabilities to the push trigger so the fork runs the full build+test matrix on each push and we can watch the result directly (the pull_request trigger only runs in the upstream PR context). Revert before merge. * fix(deps): align ts-async constraint in properties-js to < 0.6.0 'rush check' failed: @microsoft/applicationinsights-properties-js still declared '@nevware21/ts-async: >= 0.5.5 < 2.x' while every other package was narrowed to '>= 0.5.5 < 0.6.0' during the upstream/main merge. This file was not part of the merge conflict set so it was missed. Narrowed it to match. 'rush check' now reports no mis-matching deps (exit 0). * ci: revert temporary push-trigger for pnpm migration branch * revert: remove PR #2737 build/CI scope creep from pnpm migration branch Reverts commit 63c6ed7 ("apply build/CI improvements from PR 2737") which was unrelated to the pnpm migration + vulnerability remediation. Restored to upstream/main state: - gruntfile.js: removed _getPuppeteerExecutablePath() + the unit/perf puppeteer-options refactor (CI relies on puppeteer's native PUPPETEER_EXECUTABLE_PATH handling, same as upstream). Kept the pnpm-migration content (a52dd9f). - tools/grunt-tasks/minifyNames.js: removed sleepForRetry() - shared/AppInsightsCore/src/utils/DataCacheHelper.ts: #version# -> 3.4.1 - common/Tests/Framework/rollup.config.js: dropped extra updateDistEsmFiles arg - tools/shims/Tests/UnitTests.html: dropped extra modules.run arg Also reverted e021de0 (the perf-qunit syntax fix) implicitly, since the malformed block it fixed only existed because of 63c6ed7. extUtils.ts already matched upstream (#extVersion#) and was left as-is.
hectorhdzg
added a commit
that referenced
this pull request
Jun 18, 2026
…#2744) * fix(#2736): canonicalize PURE annotations in dist-es5 (module) output Modern bundlers (Rolldown / Vite 8) prefer the package "module" entry, which resolves to the per-module dist-es5 tsc output. TypeScript emits parenthesized PURE annotations with whitespace after the opening paren (e.g. `( /*#__PURE__*/"http.")`), which Rolldown rejects with [INVALID_ANNOTATION]. The existing rollup fixPureAnnotations() pass only covers the bundled dist/es5 ("main") output, so the ESM consumer path stayed broken (#2737 only fixed the bundle). Changes: - Add shared tools/pureAnnotations.js (single source of truth for the canonicalization regex), reused by both rollup.base.config.js and the new grunt task. - Add "fix-pure" grunt multitask that canonicalizes dist-es5/**/*.js after the tsc compile; wired into the shared tsBuildActions pipeline for all packages. - Extract shared getDistPackageRoots() in gruntfile.js (reused by validate-es5 and fix-pure). - Keep the wrapping parens (required for older Rollup/Webpack/Terser to tree-shake the constants), per maintainer guidance. - Extend AISKU/AppInsightsCore size tests to assert canonical PURE form in the dist-es5 module output. - Document the fix in RELEASES.md (3.4.2). * fix: load shared PURE helper as ESM (.mjs) so rollup config bundling resolves it rollup's --bundleConfigAsCjs writes the bundled config into the subpackage dir and does not run the commonjs plugin, so a relative require()/CJS import of the shared helper failed to resolve. Convert the shared helper to an ES module and import it directly in rollup.base.config.js (rollup inlines it); the CommonJS grunt task now loads it via async dynamic import().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.