-
Notifications
You must be signed in to change notification settings - Fork 3
Add exports=named to rollup output config #605
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdded the property exports: 'named' to the output configuration in both ESM and CommonJS build blocks within rollup.config.mjs. No other configuration keys or files were modified. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rollup.config.mjs (2)
17-17
: Remove redundant exports option from the ESM build.
output.exports
only affects non-ES formats (notesm
). Keeping it here is harmless but misleading; Rollup will ignore it. Consider removing to avoid confusion. (rollupjs.org)Apply this diff:
format: 'esm', - exports: 'named', entryFileNames: '[name].mjs',
11-38
: Follow-ups: package export map and subpath coverage.Given you output both ESM (.mjs) and CJS (.cjs) for multiple entry points (
index
,transform
), ensure package.json “exports” maps both conditions and subpaths, e.g.:
- "." → { import: "./dist/index.mjs", require: "./dist/index.cjs" }
- "./transform" → { import: "./dist/transform.mjs", require: "./dist/transform.cjs" }
This avoids resolution ambiguity in Node’s Node16/NodeNext moduleResolution and lets TypeScript pick the right flavor.
Would you like me to draft a package.json “exports” block and a short README note showing the new CommonJS usage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
rollup.config.mjs
(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
rollup.config.mjs (1)
src/plugin.ts (1)
generateBundle
(31-59)
🔇 Additional comments (1)
rollup.config.mjs (1)
30-30
: Please manually verify the CJS output shapeThe
dist/
directory was not present in the verification environment, so we couldn’t confirm that your CommonJS build is emittingexports.default = …
and no longer usingmodule.exports = …
. To ensure the intended breaking‐change behavior for CJS consumers, please:
Run your build (e.g.
npm run build
) so that thedist/
directory and.cjs
files exist.Confirm each
dist/*.cjs
contains at least one occurrence ofexports.default =and no occurrences of
module.exports =Optionally, add a CI check or post‐build script similar to the one below to automate this guard:
#!/usr/bin/env bash set -euo pipefail echo "Verifying CommonJS output in dist/*.cjs…" rg -n "exports\\.default\\s*=" dist/*.cjs \ || { echo "❌ Missing exports.default in CJS output"; exit 1; } ! rg -n "module\\.exports\\s*=" dist/*.cjs \ || { echo "❌ Found legacy module.exports assignment"; exit 1; } echo "✅ CJS exports are correctly in named mode with default on .default"Once confirmed, you may also want to update your
docs/CHANGELOG.md
to note that CJS consumers must switch from:require('pkg')to:
require('pkg').default
@BPScott thank you for the pr! Changing the export is a breaking change, and in the long run, we prefer to use default exports for ease of use. We are already working on fixing the support across all the export types(cjs, mjs, d.ts), will provide an update later this week. How did you encounter the error? Did you update the plugin, or did you change the type of the vite/rollup config? |
I encountered this error when I changed the tsconfig that runs over my I changed When I made this change I began to get the following type-check error on the line that did
I've created a reproduction demo for you at https://github.com/BPScott/rollup-plugin-webpack-stats-typeerror Clone the repo, and then run |
Currently index.ts contains a single default export, and as
output.exports
is not specified it uses the value of "default". This causes problems with generating CommonJS output that is meant to be interchangeable with ESM output as detailed in https://rollupjs.org/configuration-options/#output-exports and https://www.typescriptlang.org/docs/handbook/modules/appendices/esm-cjs-interop.html. This also causes problems with typescript'smoduleResolution:node16
option as the types types published by this package do match the commonjs output.Prior to this change the types in
index.d.ts
areimport { webpackStats } from './plugin'; export default webpackStats;
which is correct for the esm build output.The compiled
index.cjs
file however containsmodule.exports = webpackStats;
. The correct types for this this content would beexport = webpackStats
.To make the commonjs output match the types that are generated, and have commonjs and esm behave equivalently, this PR adds
exports: "named"
to the rollup output options.This has no effect on the esm output, but changes the commonjs output in index.cjs file as follows:
This is a breaking change, as it means commonjs consumers will have to change how they consume the package:
My general approach to working around this problem is "I only ever use named exports, I totally avoid
export default
", so personally I'd also changeexport default webpackStats
toexport {webpackStats}
but I figure this is a less invasive change.Summary by CodeRabbit