chore: migrate toolchain to TypeScript 5.5, Vite, Vitest, and ESLint v9#84
Conversation
Replaces the 2019-era ES5/Rollup/Karma/Mocha/Chai stack with a modern toolchain aligned with the internal sdk-web repo: - Source: src/Rokt-Kit.js → src/Rokt-Kit.ts (class-based, strict mode) - Build: Rollup v1 → Vite (IIFE + CJS + .d.ts via vite-plugin-dts) - Tests: Karma + Mocha/Chai → Vitest + jsdom (158 tests passing) - Lint: ESLint v7 (.eslintrc) → ESLint v9 flat config (eslint.config.mjs) - Prettier: updated to 120-char width, single quotes, trailing commas - Node: .nvmrc bumped to 24 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Background: this PR replaces both the build tool and the test runner for a browser SDK. The description lists the mechanical changes, but it does not say whether browser support changes or what replaces the old real-browser verification. Please add that context explicitly. Missing in this PR description: the supported browser baseline after the Vite build, whether dropping Chrome and Firefox coverage is intentional, and whether a browser-level smoke test is coming in a follow-up PR. -Codex |
| import { resolve } from 'path'; | ||
|
|
||
| export default defineConfig({ | ||
| build: { |
There was a problem hiding this comment.
Background: this build now ships a materially different browser target from the current release. The emitted dist/Rokt-Kit.iife.js contains const, let, class, arrow functions and ??, while the previous bundle was ES5-style. For a kit that is injected into merchant pages, that is a breaking runtime change unless we have explicitly raised the browser baseline.
Please set an explicit supported build target and verify the published IIFE against it before merging.
-Codex
There was a problem hiding this comment.
Done — added target: 'es2020' to vite.config.ts. The IIFE now has an explicit browser baseline. Added a note to the PR description clarifying this is the first explicit target (no baseline existed before).
| "build": "vite build", | ||
| "lint": "eslint \"src/**/*.ts\" \"test/**/*.ts\"", | ||
| "lint:fix": "eslint \"src/**/*.ts\" \"test/**/*.ts\" --fix", | ||
| "test": "vitest run", |
There was a problem hiding this comment.
Background: after this change the executed suite is test/src/**/*.spec.ts, but the old Karma harness still stays in-tree: test/src/tests.js, test/index.html, test/config.js and test/lib/mockhttprequest.js. That leaves a second, dead test path in the repo and adds maintenance cost to a migration PR that is supposed to simplify the toolchain.
Delete the legacy harness in the same PR, or call out clearly why it needs to stay.
-Codex
There was a problem hiding this comment.
Done — deleted test/src/tests.js, test/index.html, test/config.js, and test/lib/mockhttprequest.js in commit 7d03487.
src/Rokt-Kit.ts
Outdated
|
|
||
| const selection = this.launcher!.selectPlacements(selectPlacementsOptions); | ||
|
|
||
| // After selection resolves, sync the Rokt session ID back to mParticle |
There was a problem hiding this comment.
Background: this works, but the same side-effect is repeated in four branches, which makes the success and failure flow harder to scan than it needs to be. Normalising the result to one promise and logging once in a terminal branch would read more cleanly.
For example:
const logSelection = () => this.logSelectPlacementsEvent(selectPlacementsAttributes);
void Promise.resolve(selection)
.then((sel) => sel?.context?.sessionId?.then((sessionId) => this.setRoktSessionId(sessionId)))
.catch(() => undefined)
.finally(logSelection);That keeps the monadic flow in one place and makes the side-effects explicit.
-Codex
There was a problem hiding this comment.
Done — adopted the single Promise.resolve() chain with .finally() for the log, as suggested. Fixed in 7d03487.
- Set explicit build target (es2020) in vite.config.ts - Refactor selectPlacements to use a single functional promise chain - Delete legacy Mocha HTML test harness (tests.js, index.html, config.js, mockhttprequest.js) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Re: @mattbodle's comment: The old test harness was a manual Mocha HTML page ( |
| getUserIdentities?: () => { userIdentities: Record<string, string> }; | ||
| } | ||
|
|
||
| interface KitFilters { |
There was a problem hiding this comment.
Nit: Can this be imported from the core SDK type definitions?
There was a problem hiding this comment.
I don't see how they are exported from the core SDK - are they published separately somewhere?
There was a problem hiding this comment.
There was a problem hiding this comment.
we are going to create all of these into the core sdk, then pull the types from the core sdk. so for now i'd ignore. we'll pull these in in the future
| filteredUser?: FilteredUser | null; | ||
| } | ||
|
|
||
| interface RoktManager { |
There was a problem hiding this comment.
Nit: Can this be imported from the core SDK type definitions?
…vitest-migration # Conflicts: # src/Rokt-Kit.js # test/src/tests.js
- Rename KitSettings → RoktKitSettings - Replace mergeObjects() with spread operators throughout - Remove non-null assertions on window.Rokt.__event_stream__ calls - Make EventDataType and EventCategory required in MParticleEvent; remove ?? 0 fallbacks - Add MESSAGE_TYPE_PROFILE constant to replace magic number 14 - Add ESM build output (vite.config.ts formats + package.json module/exports/files) - Remove test:ui script (vitest --ui is optional dev tooling, not needed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Port of efb8905 from main into TypeScript. Adds ErrorReportingService, LoggingService, ReportingTransport, and RateLimiter classes with full test coverage (42 new tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolves modify/delete conflicts by keeping src/Rokt-Kit.js and test/src/tests.js deleted — replaced by their TypeScript equivalents in this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| getUserIdentities?: () => { userIdentities: Record<string, string> }; | ||
| } | ||
|
|
||
| interface KitFilters { |
There was a problem hiding this comment.
alexs-mparticle
left a comment
There was a problem hiding this comment.
LGTM. Once @rmi22186's comments are approved, I think we can merge this.
# [1.23.0](v1.22.0...v1.23.0) (2026-04-08) ### Features * migrate toolchain to TypeScript 5.5, Vite, Vitest, and ESLint v9 ([#84](#84)) ([58f6b41](58f6b41))
Summary
src/Rokt-Kit.js(ES5) tosrc/Rokt-Kit.ts(TypeScript 5.5, strict mode, class-based)target: 'es2020') producing IIFE + CJS bundles +.d.tstype declarationstest/index.html) with Vitest + jsdom (158 tests, headless — no automated real-browser testing existed before).eslintrcwith ESLint v9 flat config (eslint.config.mjs).nvmrcto Node 24npm run build:watchfor local development (Vite watch mode)Browser baseline: The emitted IIFE targets ES2020 (
const,let,class,??,?.). This is the first explicit browser target — no baseline existed before. Real-browser smoke testing can be added in a follow-up if needed.Test plan
npm run lint— zero errorsnpm run build— producesdist/Rokt-Kit.iife.js,dist/Rokt-Kit.common.js,dist/Rokt-Kit.d.tsnpm run build:watch— watches for changes and rebuildsnpm run test— 158/158 tests pass in Vitest/jsdom🤖 Generated with Claude Code