chore(shared): replace telemetry postinstall with runtime notice#8549
Open
jacekradko wants to merge 3 commits into
Open
chore(shared): replace telemetry postinstall with runtime notice#8549jacekradko wants to merge 3 commits into
jacekradko wants to merge 3 commits into
Conversation
Move the one-time telemetry disclosure off of npm lifecycle scripts and into the TelemetryCollector itself. Surfacing it at runtime instead of install time keeps @clerk/shared free of postinstall code, drops the std-env dependency, and only shows the notice in environments where telemetry actually fires. The Node persistence marker reuses the prior env-paths location, so users who already saw the notice via postinstall will not see it again. Resolves SDK-84.
🦋 Changeset detectedLatest commit: 8d23466 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe patch removes the install-time Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Member
|
@jacekradko my hunch is that this is going to trigger more than desired in the browser, and we'll get requests to disable the warning altogether similar to what happened with the dev mode warning. How can we get ahead of that? 🤔 |
@clerk/ui re-bundles @clerk/shared with Rspack, which does not handle bare
`node:` URI imports even when they are only reachable on a Node runtime.
Route the dynamic imports through a `new Function('id', 'return import(id)')`
indirection so the module specifiers are no longer visible to static
analysis. Also apply prettier to the new spec file.
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Next.js Edge Runtime forbids `new Function`/`eval` even in unreachable code, so the dynamic-import-evasion trick used to hide `node:` module specifiers from webpack tripped the edge analyzer when @clerk/shared was reached through @clerk/nextjs middleware. Replace the Node config-file persistence with a `globalThis` Symbol flag (browser still uses localStorage). The notice now shows at most once per process on Node and once per machine in the browser, with no filesystem access and no dynamic-code APIs anywhere in the bundle.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/shared/src/telemetry/notice.ts`:
- Around line 66-75: hasSeen() currently returns early if
hasUsableLocalStorage() is true even when localStorage.getItem throws, so the
process-level dedupe (PROCESS_FLAG) never gets consulted; update hasSeen() to
attempt reading localStorage in a try/catch and on exception fall back to
returning the boolean value of (globalThis as Record<symbol,
unknown>)[PROCESS_FLAG] instead of false. Also adjust markSeen() so when writing
to localStorage throws it sets PROCESS_FLAG as the fallback; reference
STORAGE_KEY, PROCESS_FLAG, hasUsableLocalStorage(), hasSeen(), and markSeen()
when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f335de7c-4216-4858-aabd-2ebf30fd08e7
📒 Files selected for processing (4)
.changeset/sdk-84-remove-telemetry-postinstall.mdpackages/shared/src/__tests__/telemetry.notice.spec.tspackages/shared/src/telemetry/collector.tspackages/shared/src/telemetry/notice.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/sdk-84-remove-telemetry-postinstall.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/shared/src/telemetry/collector.ts
- packages/shared/src/tests/telemetry.notice.spec.ts
Comment on lines
+66
to
+75
| function hasSeen(): boolean { | ||
| if (hasUsableLocalStorage()) { | ||
| try { | ||
| return globalThis.localStorage.getItem(STORAGE_KEY) === '1'; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
| return Boolean((globalThis as Record<symbol, unknown>)[PROCESS_FLAG]); | ||
| } |
Contributor
There was a problem hiding this comment.
Process-level dedupe is skipped when localStorage exists but cannot persist/read reliably.
On Line 66–72, hasSeen() returns from the localStorage branch and never checks PROCESS_FLAG. On Line 77–87, markSeen() sets PROCESS_FLAG as fallback, but that fallback is effectively ignored on later calls when localStorage is present. In restricted storage contexts, the notice can print repeatedly instead of once.
Suggested fix
function hasSeen(): boolean {
+ if (Boolean((globalThis as Record<symbol, unknown>)[PROCESS_FLAG])) {
+ return true;
+ }
+
if (hasUsableLocalStorage()) {
try {
return globalThis.localStorage.getItem(STORAGE_KEY) === '1';
} catch {
return false;
}
}
- return Boolean((globalThis as Record<symbol, unknown>)[PROCESS_FLAG]);
+ return false;
}
function markSeen(): void {
+ // Always dedupe within the current process, regardless of storage state.
+ (globalThis as Record<symbol, unknown>)[PROCESS_FLAG] = true;
+
if (hasUsableLocalStorage()) {
try {
globalThis.localStorage.setItem(STORAGE_KEY, '1');
- return;
} catch {
- // fall through to the in-process flag
+ // ignore storage failures; in-process dedupe is already set
}
}
- (globalThis as Record<symbol, unknown>)[PROCESS_FLAG] = true;
}Also applies to: 77-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/telemetry/notice.ts` around lines 66 - 75, hasSeen()
currently returns early if hasUsableLocalStorage() is true even when
localStorage.getItem throws, so the process-level dedupe (PROCESS_FLAG) never
gets consulted; update hasSeen() to attempt reading localStorage in a try/catch
and on exception fall back to returning the boolean value of (globalThis as
Record<symbol, unknown>)[PROCESS_FLAG] instead of false. Also adjust markSeen()
so when writing to localStorage throws it sets PROCESS_FLAG as the fallback;
reference STORAGE_KEY, PROCESS_FLAG, hasUsableLocalStorage(), hasSeen(), and
markSeen() when making these changes.
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
2 participants
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.
NOT READY FOR REVIEW YET!
Moves the one-time telemetry disclosure off the
postinstalllifecycle script and into theTelemetryCollectoritself. The same notice now fires at runtime the first time telemetry activates on a development instance, gated on CI, headless, and the existing opt-out signals. The Node persistence marker reuses the prior env-paths location so anyone who already saw the postinstall notice will not see it again. Drops thestd-envdependency and removes install-time code from the published package, which addresses the supply-chain concern raised in #8540.Resolves SDK-84.