Skip to content

Conversation

@daniel-graham-amplitude
Copy link
Collaborator

@daniel-graham-amplitude daniel-graham-amplitude commented Dec 24, 2025

Summary

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

Note

Tooling/CI

  • Replace Yarn with pnpm across GitHub Actions, Husky hooks, and docs; add pnpm setup/caching and upgrade actions/setup-node to v4
  • Root package.json now declares packageManager: pnpm; all package scripts updated to use pnpm

Workspace/deps

  • Switch many internal dependencies to workspace:* and update tests/imports to use @amplitude/analytics-core instead of deprecated packages

Core/API surface

  • Add and export InstanceProxy and related types via new analytics-core/src/types/proxy.ts; re-export in analytics-core/src/index.ts

Tests/config

  • Add test tsconfig.json with Jest/Node types to several packages; adjust React Native Jest transformIgnorePatterns for pnpm

Session Replay

  • Type fixes: make getDebugConfig return SessionReplayJoinedConfig; parse string sessionId to number in targeting flow with test coverage

Workflows

  • Update ci, ci-nx, docs, and e2e workflows to use pnpm for install/build/test and docs generation

Written by Cursor Bugbot for commit 93b19e5. This will update automatically on new commits. Configure here.

@daniel-graham-amplitude daniel-graham-amplitude marked this pull request as draft December 24, 2025 23:30
@macroscopeapp
Copy link
Contributor

macroscopeapp bot commented Dec 24, 2025

Migrate the repository to PNPM and update CI workflows, Husky hooks, workspace config, and internal package references to use PNPM 10.26.1 with setup-node v4 caching

Switch build, test, lint, and docs commands from yarn to pnpm across CI and packages, add pnpm-workspace.yaml and pnpm-lock.yaml, update actions/setup-node to v4 with PNPM cache, adjust Jest configs for .pnpm, export InstanceProxy from @amplitude/analytics-core, and fix SessionReplay targeting to parse string sessionId to number with a test.

🖇️ Linked Issues

Resolves AMP-145286 under the AMP-145285 epic and supports the AMP-91393 initiative by migrating the monorepo to PNPM.

📍Where to Start

Start with the workspace and CI entry points: pnpm-workspace.yaml, package.json, and CI workflows in .github/workflows/ci.yml and .github/workflows/ci-nx.yml; then review the InstanceProxy export in packages/analytics-core/src/index.ts and the targeting fix in packages/session-replay-browser/src/targeting/targeting-manager.ts.


Macroscope summarized 93b19e5. (Automatic summaries will resume when PR exits draft mode or review begins).

Comment on lines 42 to 48
const targetingResult = await evaluateTargetingPackage({
...targetingParams,
flag: targetingConfig,
sessionId: sessionId,
sessionId: typeof sessionId === 'string' ? parseInt(sessionId, 10) : sessionId,
apiKey: apiKey,
loggerProvider: loggerProvider,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Guard values before parseInt or substring to avoid NaN and runtime errors; skip/omit when inputs are invalid.

-    const targetingResult = await evaluateTargetingPackage({
-      ...targetingParams,
-      flag: targetingConfig,
-      sessionId: typeof sessionId === 'string' ? parseInt(sessionId, 10) : sessionId,
-      apiKey: apiKey,
-      loggerProvider: loggerProvider,
-    });
+    const parsedSessionId = typeof sessionId === 'string' ? Number.parseInt(sessionId, 10) : sessionId;
+    const targetingResult = await evaluateTargetingPackage({
+      ...targetingParams,
+      flag: targetingConfig,
+      ...(typeof parsedSessionId === 'number' && !Number.isNaN(parsedSessionId) ? { sessionId: parsedSessionId } : {}),
+      apiKey: apiKey,
+      loggerProvider: loggerProvider,
+    });

🚀 Want me to fix this? Reply ex: "fix it for me".

- name: Lint all packages
run: |
yarn lint
pnpm lint
Copy link

Choose a reason for hiding this comment

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

Build action uses pnpm without installing it first

The composite action was updated to use pnpm install, pnpm build, pnpm test, and pnpm lint commands, but no step was added to install pnpm (using pnpm/action-setup@v4). The publish-v2.yml workflow uses this action without setting up pnpm first, which will cause the publish workflow to fail with "command not found" errors. Other workflows like ci.yml and ci-nx.yml were correctly updated to include the pnpm setup step, but this action was not.

Fix in Cursor Fix in Web

yarn build:vite
yarn start --port 5173 &
pnpm build:vite
pnpm start --port 5173 &
Copy link
Contributor

Choose a reason for hiding this comment

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

pnpm won’t pass --port to the start script without --. Consider pnpm start -- --port 5173 so the arg reaches the app.

Suggested change
pnpm start --port 5173 &
pnpm start -- --port 5173 &

🚀 Want me to fix this? Reply ex: "fix it for me".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants