-
Notifications
You must be signed in to change notification settings - Fork 58
chore: start migration from Yarn -> PNPM #1443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| const targetingResult = await evaluateTargetingPackage({ | ||
| ...targetingParams, | ||
| flag: targetingConfig, | ||
| sessionId: sessionId, | ||
| sessionId: typeof sessionId === 'string' ? parseInt(sessionId, 10) : sessionId, | ||
| apiKey: apiKey, | ||
| loggerProvider: loggerProvider, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sessionId can become NaN when the input string is non‑numeric, and that NaN is passed to evaluateTargetingPackage. Consider guarding the parse and omit or handle sessionId when it cannot be parsed to avoid evaluating with NaN.
- const targetingResult = await evaluateTargetingPackage({
- ...targetingParams,
- flag: targetingConfig,
- sessionId: typeof sessionId === 'string' ? parseInt(sessionId, 10) : sessionId,
- apiKey: apiKey,
- loggerProvider: loggerProvider,
- });
+ const parsedSessionId = typeof sessionId === 'string' ? parseInt(sessionId, 10) : sessionId;
+ const targetingResult = await evaluateTargetingPackage({
+ ...targetingParams,
+ flag: targetingConfig,
+ ...(typeof parsedSessionId === 'number' && !Number.isNaN(parsedSessionId) ? { sessionId: parsedSessionId } : {}),
+ apiKey: apiKey,
+ loggerProvider: loggerProvider,
+ });🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Migrate repo tooling and CI from Yarn to PNPM and fix
|
This reverts commit a7cdb0a.
Mercy811
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @daniel-graham-amplitude. LGTM! Merging to a feature branch is a good idea
…cript into AMP-145286-use-pnpm
4a01050
into
pnpm-migration
Summary
(if you exclude lockfiles, the lines of code difference is about +300 and -200)
Still to do:
Checklist
Note
Migrates the monorepo to pnpm and aligns internal dependencies and typings.
setup-node@v4, addpnpm/action-setup, enablecache: 'pnpm', and swap all yarn commands forpnpminci.yml,ci-nx.yml,e2e.yml,docs.yml, and the compositeBuild and TestactionpackageManagerset to pnpmdependenciestoworkspace:*(e.g., analytics-browser, plugins, node/react-native test pkgs) to ensure consistent linking@amplitude/analytics-types/analytics-client-commonto@amplitude/analytics-core; core now exportsInstanceProxy; update browser snippet typings and plugin/tests accordinglytransformIgnorePatternsfor pnpm hoisting and per-package testtsconfig.jsonsessionIdto number in targeting evaluation and add tests; type tweak ongetDebugConfig; add@amplitude/experiment-coreandrrweb-related depssetTransportand importTransportTypeOrOptions@amplitude/rrwebWritten by Cursor Bugbot for commit add4784. This will update automatically on new commits. Configure here.