Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA plugin block responsible for automatically generating an ESM-specific package.json file during the build bundling process has been removed from the TypeScript build configuration. The ESM externalization behavior remains unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the shared tsdown JS library build config by removing the plugin that emits a package.json into the ESM output directory.
Changes:
- Removed the
generateBundleplugin that wrotedist/esm/package.jsonwith{ "type": "module" }.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR removes the Key observations:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Next.js App
participant BundlerESM as Bundler (import condition)
participant BundlerCJS as Bundler (require condition)
participant ESMBuild as dist/esm/index.js<br/>(ES module syntax)
participant CJSBuild as dist/index.js<br/>(CommonJS)
participant NodeESM as Node.js ESM Loader<br/>(no bundler)
Note over App: With plugin (before this PR)
App->>BundlerESM: import "@stackframe/stack"
BundlerESM->>ESMBuild: resolves via "import" condition
Note over ESMBuild: dist/esm/package.json<br/>{"type":"module"} present
ESMBuild-->>App: ESM module instance A (own React context)
App->>BundlerCJS: require("@stackframe/stack")
BundlerCJS->>CJSBuild: resolves via "require" condition
CJSBuild-->>App: CJS module instance B (different React context)
Note over App: ⚠️ Dual-module hazard: useStackApp from B<br/>cannot see StackProvider from A → infinite loop
Note over App: Without plugin (after this PR)
App->>BundlerESM: import "@stackframe/stack"
BundlerESM->>ESMBuild: still resolves to dist/esm/index.js
Note over ESMBuild: No dist/esm/package.json<br/>Bundler parses ESM content directly → OK
ESMBuild-->>App: bundler handles it, but dual-module hazard may persist
App->>NodeESM: import "@stackframe/stack" (no bundler)
NodeESM->>ESMBuild: resolves via "default" condition
Note over ESMBuild: No {"type":"module"} → Node.js treats as CJS
ESMBuild-->>NodeESM: ❌ SyntaxError: unexpected export keyword
Last reviewed commit: 96dfbab |
| external: true, | ||
| }; | ||
| }, | ||
| }, | ||
| { | ||
| name: 'stackframe: write esm package.json', | ||
| generateBundle(options) { | ||
| if (options.dir?.endsWith('/esm') || options.dir === 'dist/esm') { | ||
| this.emitFile({ | ||
| type: 'asset', | ||
| fileName: 'package.json', | ||
| source: JSON.stringify({ type: 'module' }) + '\n', | ||
| }); | ||
| } | ||
| }, | ||
| }, | ||
| } | ||
| ], |
There was a problem hiding this comment.
Removing the ESM package.json risks breaking Node.js-native ESM consumers
The removed plugin wrote {"type":"module"} into dist/esm/package.json at build time. That file is still listed as the ESM entry point in the package exports map:
// packages/stack/package.json
"import": { "default": "./dist/esm/index.js" }
// packages/stack-shared/package.json
"default": "./dist/esm/index.js"The dist/esm/*.js files still contain ES module export/import syntax (produced by tsdown's format.esm). Without the {"type":"module"} marker, Node.js's native ESM loader will attempt to parse them as CommonJS and throw a SyntaxError: Cannot use import statement in a module (or similar) for any caller that reaches dist/esm/ files without an intermediate bundler — for example, Node.js scripts that do import "@stackframe/stack" directly, or test runners that load the package in Node.js native ESM mode.
This also doesn't address the true root cause of the React context loss: the classic dual-package hazard where both dist/esm/ and dist/ are in memory simultaneously with separate module-level singletons (including separate React.createContext instances). A more targeted fix would be:
- Deduplicating the context object outside the dual build (e.g., with
globalThis-based singleton fallback), OR - Dropping the dual-build in favour of a single CJS build (as the previous tsup config did), OR
- Changing the
dist/esm/*.jsfile extension to.mjsto make the module format unambiguous without a directory-levelpackage.json.
Context
On preview branches, we were running into an issue where once you hit the handler/sign-in it kept refreshing and looping if you weren't signed in already. This occurred because there were two versions of stack-provider-client that were being generated and the StackProvider was providing context for one version, and the useStackApp was trying to consume context from the other version. This was caused by this PR, due to the changes to
js-library.ts(adding the new plugin).