-
Notifications
You must be signed in to change notification settings - Fork 419
fix: Improve ESM compatibility for named exports #2724
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
fix: Improve ESM compatibility for named exports #2724
Conversation
This commit fixes ESM interoperability issues identified by @arethetypeswrong/cli
where named exports weren't accessible when importing from ESM.
Changes:
- Refactor receiver exports to use import-then-export pattern instead of
`export { default as X }` syntax
- Separate type-only exports using `export type` for better tree-shaking
- Fix Logger and OAuth type re-exports to be type-only
The previous pattern `export { default as ExpressReceiver } from './receivers/ExpressReceiver'`
generated code with `__importDefault().default` which isn't statically analyzable
by Node.js's ESM-CJS interop layer.
The new pattern imports first then re-exports:
```typescript
import ExpressReceiver from './receivers/ExpressReceiver';
export { ExpressReceiver };
export type { ExpressReceiverOptions } from './receivers/ExpressReceiver';
```
This generates simpler CommonJS code that Node.js can properly expose as named
exports when imported from ESM.
Verified with @arethetypeswrong/cli - NamedExports errors are now resolved.
Relates to #2565, #2632, #2644
|
Thanks for the contribution! Before we can merge this, we need @grantjoy to sign the Salesforce Inc. Contributor License Agreement. |
|
Hi @grantjoy 👋🏻 Thanks for taking the time to craft an in-depth and quality pull request! 👌🏻 The PR looks safe to run our GitHub Workflows, so I've approved it. It looks like there are some minor failures due to biomjs formatting: ❓ When you have a moment, would you mind pushing a commit to fix the formatting error? It looks like the newline on L95 should be removed. 🔍 I'll ping a few teammates to look over your pull request, but on first review it's look good to move forward! 👍🏻 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2724 +/- ##
=======================================
Coverage 93.44% 93.44%
=======================================
Files 37 37
Lines 7675 7675
Branches 669 669
=======================================
Hits 7172 7172
Misses 498 498
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
zimeg
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.
@grantjoy LGTM! Thanks for bringing this enhancement to exports - I agree this is a nice pattern to use 🙏 ✨
Summary
This PR fixes ESM interoperability issues where named exports (particularly
ExpressReceiver,SocketModeReceiver,HTTPReceiver, andAwsLambdaReceiver) weren't accessible when importing from ESM, as identified by@arethetypeswrong/cli.Problem
The current export pattern:
Generates CommonJS code like:
This dynamic property access pattern isn't statically analyzable by Node.js's ESM-CJS interop layer, so these exports don't get exposed when importing from ESM:
Solution
Refactored to use an import-then-export pattern:
This generates simpler, statically analyzable CommonJS code that Node.js can properly expose as named exports in ESM.
Additionally:
export typefor better tree-shakingLoggerand OAuth type re-exports to be type-only (they're interfaces, not runtime values)Verification
Before:
{ "problems": { "NamedExports": [ { "missing": [ "Logger", "ExpressReceiver", "SocketModeReceiver", "HTTPReceiver", "AwsLambdaReceiver", "Installation", "InstallURLOptions", "InstallationQuery", "InstallationStore", "StateStore", "InstallProviderOptions" ] } ] } }After:
$ bunx @arethetypeswrong/cli --pack . -f json{ "problems": {} }✅ All
NamedExportsissues resolved!ESM Import Test:
CommonJS Compatibility:
Tests
All existing tests pass:
Related Issues
Migration Guide
This change is fully backward compatible. No code changes required for consumers.
Both import styles continue to work: