chore(shared): replace telemetry postinstall with runtime notice#8549
Open
jacekradko wants to merge 6 commits into
Open
chore(shared): replace telemetry postinstall with runtime notice#8549jacekradko wants to merge 6 commits into
jacekradko wants to merge 6 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: 9e5fe52 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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe postinstall telemetry script and the 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
Member
Author
|
@brkalow I think you are right. It's tricky in the browser. Are we ok with this only on the server? |
Browser consoles are observed by non-developers (QA, screenshots, demos), so adding another runtime warning there invites the same complaints we get about the existing dev-mode banner. Drop the browser surface entirely and emit the disclosure only on Node, matching the noise profile of the original postinstall (terminal-only, dev-eyes-only). Persistence simplifies to a single per-process globalThis flag; the localStorage path and the bug it had around getItem exceptions both go away.
The Next.js Edge Runtime build-time analyzer flags any reachable read of process.versions even when guarded — this caused a warning when @clerk/backend was reached through middleware, which the next-build integration test asserts against. Detect Edge Runtime via its standard `EdgeRuntime` global instead, and use the absence of `window`+`EdgeRuntime` as the positive signal for "server runtime" without touching process.versions at all.
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.