Skip to content

Conversation

@nlynzaad
Copy link
Contributor

@nlynzaad nlynzaad commented Oct 1, 2025

currently we are exporting the source ts files from e2eUtils.

this is not a problem in 99% of cases but we are getting errors every now and then where the tests suites are unable to resolve the paths in the e2eUtils package.

this PR adds a built step, using tsUp, to the e2eUtils, bundling all into index.js, and updates the package exports to point to these built files.

Summary by CodeRabbit

  • Chores
    • Package now ships built ESM and CommonJS artifacts with TypeScript declarations, updated exports, packaging metadata (files, sideEffects, engines), and a build script to produce distributables; build config updated to emit both ESM and CJS and generate declaration files; added a declaration-generation dev tool.
  • Tests
    • Improved e2e redirect test reliability by waiting for DOM content to load after navigation when a full document reload occurs.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Walkthrough

Reworks e2e-utils packaging and build: adds Vite build config, generates ESM/CJS outputs and d.ts files, and updates package.json exports/metadata. Adjusts an E2E React Router test to wait for domcontentloaded after URL change when reloadDocument is true.

Changes

Cohort / File(s) Summary of Changes
e2e-utils package & build config
e2e/e2e-utils/package.json, e2e/e2e-utils/vite.config.ts
Adds a build script (vite build); introduces top-level types, main, module, sideEffects, files, and engines fields; updates exports to provide structured import and require entries pointing to dist/esm/dist/cjs artifacts and types; adds vite-plugin-dts to devDependencies; replaces merged/tanstack config with a standalone defineConfig Vite build config that emits ESM and CJS with preserved modules and uses vite-plugin-dts to output declarations to dist/types.
E2E test redirect flow
e2e/react-router/basic-file-based/tests/redirect.spec.ts
After await page.waitForURL(url), conditionally awaits page.waitForLoadState('domcontentloaded') when reloadDocument is truthy, adding an extra wait in that branch.

Sequence Diagram(s)

sequenceDiagram
  participant Dev as Developer/CI
  participant Vite as Vite Build
  participant Rollup as Rollup (via Vite)
  participant DTS as vite-plugin-dts
  participant Dist as dist/

  Dev->>Vite: run `vite build`
  Vite->>Rollup: bundle with SSR and preserveModules
  Rollup->>Dist: write ESM to ./dist/esm and CJS to ./dist/cjs
  Vite->>DTS: generate type declarations (entryRoot: ./src)
  DTS->>Dist: write types to ./dist/types (copy dts files)
  Dev-->>Dist: packaged artifacts ready (ESM, CJS, types)
Loading
sequenceDiagram
  actor Tester
  participant Test as Redirect Test
  participant Page as Playwright Page

  Tester->>Test: run test with reloadDocument flag
  Test->>Page: navigate / trigger redirect
  Page-->>Test: URL changes
  Test->>Page: waitForURL(target)
  alt reloadDocument == true
    Note over Test,Page: Conditional additional wait
    Test->>Page: waitForLoadState('domcontentloaded')
  end
  Test-->>Tester: proceed with assertions
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

package: start-plugin-core, package: start-server-core

Poem

I hop through src, then out to dist,
types in tow, none gone amiss.
A redirect pauses—domcontent seen,
I twitch my nose where builds convene.
Crumbs to dist, my paws are kissed. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary change of exporting the built e2eUtils package and corresponds directly to the modifications in build scripts and package exports, making it clear to reviewers at a glance.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch e2eutils-build-step

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f40f802 and 5c900ca.

📒 Files selected for processing (2)
  • e2e/e2e-utils/package.json (1 hunks)
  • e2e/e2e-utils/vite.config.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/package.json

📄 CodeRabbit inference engine (AGENTS.md)

Use workspace:* protocol for internal dependencies in package.json files

Files:

  • e2e/e2e-utils/package.json
e2e/**

📄 CodeRabbit inference engine (AGENTS.md)

Store end-to-end tests under the e2e/ directory

Files:

  • e2e/e2e-utils/package.json
  • e2e/e2e-utils/vite.config.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • e2e/e2e-utils/vite.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Preview
  • GitHub Check: Test
🔇 Additional comments (8)
e2e/e2e-utils/package.json (4)

11-13: LGTM! Top-level fields are correctly aligned.

The types, main, and module fields now correctly point to the build outputs that will be generated by the Vite configuration, with consistent paths matching the exports section.


16-23: LGTM! Export paths are correctly configured.

The dual-format exports are properly configured with correct paths:

  • ESM output points to ./dist/esm/index.js
  • CJS output points to ./dist/cjs/index.cjs (correctly using .cjs extension)
  • Type definitions are consistently referenced in both conditions

This matches the output configuration in vite.config.ts.


27-34: LGTM! Packaging metadata is appropriate.

The metadata correctly declares:

  • sideEffects: false for tree-shaking optimization
  • files array includes both dist (built artifacts) and src (source for sourcemaps)
  • Node.js engine requirement is reasonable

6-6: No pretest hook needed. The test:e2e target in nx.json has dependsOn: ['^build'], so e2e-utils is built automatically before running e2e tests.

e2e/e2e-utils/vite.config.ts (4)

1-6: LGTM! ESM-compatible path resolution.

The imports and __dirname variable are correctly set up for ES module compatibility:

  • Using node: protocol for Node.js built-ins
  • Properly converting import.meta.url to a directory path with fileURLToPath
  • defineConfig is correctly imported from vite (not vitest/config)

14-30: LGTM! Dual-format output configuration is correct.

The Rollup configuration properly generates both ESM and CJS outputs:

  • ESM: ./dist/esm with .js extension
  • CJS: ./dist/cjs with .cjs extension (correctly matching package.json)
  • Both use preserveModules: true to maintain source structure
  • preserveModulesRoot correctly set to 'src' directory for clean output paths

The output paths and filenames are now fully aligned with the package.json exports.


32-38: LGTM! DTS plugin configuration is appropriate.

The vite-plugin-dts configuration correctly:

  • Copies declaration files with copyDtsFiles: true
  • Sets entry root to './src' matching the source structure
  • Outputs to './dist/types' matching the package.json types paths

This will generate type definitions that align with the package exports.

Based on learnings: vite-plugin-dts 4.5.4 supports these options and is the current stable 4.x release.


10-10: Keep build.ssr: true
e2e-utils relies on Node-only APIs (node:http, node:fs), so SSR build mode is appropriate for this package.


Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Oct 1, 2025

View your CI Pipeline Execution ↗ for commit 5c900ca

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 1m 14s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-02 01:21:51 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 1, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5336

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5336

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5336

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5336

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5336

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5336

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5336

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5336

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5336

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5336

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5336

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5336

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5336

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5336

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5336

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5336

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5336

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5336

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5336

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5336

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5336

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5336

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5336

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5336

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5336

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5336

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5336

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5336

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5336

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5336

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5336

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5336

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5336

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5336

commit: 5c900ca

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
e2e/e2e-utils/package.json (1)

22-23: Add files field and prepare script to e2e-utils package.json

  • e2e/e2e-utils/package.json is missing a files field; add "files": ["dist"] to explicitly control published assets.
  • No prepare or prepack script exists; include e.g. "prepare": "pnpm build" under scripts to build before publishing.
  • dist/ is already ignored globally in root .gitignore (line 13), so no further ignore changes are needed.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f11c4c6 and 77aabef.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • e2e/e2e-utils/package.json (1 hunks)
  • e2e/react-router/basic-file-based/tests/redirect.spec.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • e2e/react-router/basic-file-based/tests/redirect.spec.ts
e2e/**

📄 CodeRabbit inference engine (AGENTS.md)

Store end-to-end tests under the e2e/ directory

Files:

  • e2e/react-router/basic-file-based/tests/redirect.spec.ts
  • e2e/e2e-utils/package.json
**/package.json

📄 CodeRabbit inference engine (AGENTS.md)

Use workspace:* protocol for internal dependencies in package.json files

Files:

  • e2e/e2e-utils/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Test
🔇 Additional comments (2)
e2e/react-router/basic-file-based/tests/redirect.spec.ts (1)

67-69: LGTM! Timing fix for reloadDocument scenario.

The conditional wait for domcontentloaded when reloadDocument is true ensures the page has fully loaded before proceeding with assertions. This addresses potential race conditions where the test might check fullPageLoad (line 72) before the domcontentloaded event fires.

e2e/e2e-utils/package.json (1)

13-16: Exports configuration is correct for dual-package (CJS + ESM).

The exports properly support both CommonJS (via require) and ESM (via import), with TypeScript type definitions included. This aligns with the PR objective of exporting built artifacts instead of source TS files.

@nlynzaad nlynzaad requested a review from SeanCassiere October 1, 2025 23:42
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

♻️ Duplicate comments (1)
e2e/e2e-utils/package.json (1)

4-9: Consider adding a pretest script to ensure build runs before tests.

Based on past review comments, tests may fail if the dist/ directory doesn't exist. Adding a pretest script ensures the package is built before tests run.

Consider this addition:

   "scripts": {
     "clean": "rimraf ./dist && rimraf ./coverage",
     "build": "vite build",
+    "pretest": "npm run build",
     "test:eslint": "eslint ./src",
     "test:unit": "exit 0; vitest --typecheck"
   },

Additionally, verify that CI workflows include a build step for the e2e-utils package before running tests.

🧹 Nitpick comments (1)
e2e/e2e-utils/package.json (1)

28-30: Remove src from published files.

The PR objective states the intent is to export built files instead of source TypeScript. Including "src" in the files array publishes the source code, which contradicts the goal and increases package size.

Apply this diff:

   "files": [
-    "dist",
-    "src"
+    "dist"
   ],
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 77aabef and f40f802.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • e2e/e2e-utils/package.json (1 hunks)
  • e2e/e2e-utils/vite.config.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • e2e/e2e-utils/vite.config.ts
e2e/**

📄 CodeRabbit inference engine (AGENTS.md)

Store end-to-end tests under the e2e/ directory

Files:

  • e2e/e2e-utils/vite.config.ts
  • e2e/e2e-utils/package.json
**/package.json

📄 CodeRabbit inference engine (AGENTS.md)

Use workspace:* protocol for internal dependencies in package.json files

Files:

  • e2e/e2e-utils/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Preview
  • GitHub Check: Test
🔇 Additional comments (2)
e2e/e2e-utils/package.json (2)

6-6: Build script looks good.

The vite build command correctly uses the new vite.config.ts to generate both ESM and CJS outputs with type definitions.


12-12: Align main field with CJS output filename.

Similar to the require export, the main field should point to the actual CJS output. After fixing the require export to use .cjs, update this field as well.

Apply this diff:

-  "main": "dist/cjs/index.cjs",
+  "main": "dist/cjs/index.cjs",

Note: If you fix the vite.config.ts to output .js instead of .cjs for CJS builds, then this field and the require export would be correct as-is. Choose one consistent approach.

Likely an incorrect or invalid review comment.

@nlynzaad nlynzaad merged commit 4b0004c into main Oct 2, 2025
7 of 8 checks passed
@nlynzaad nlynzaad deleted the e2eutils-build-step branch October 2, 2025 01:22
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.

3 participants