fix: handle specifier as default

[sxzz]

Add module.exports = exports.default; for case:

export { x as default } from './foo'
export { x as default }
export { }
export type A = string
export { type A as B }

[alangpierce]

Thank you! Agreed that this fixes the issues in your description and test, but it looks like splitting up the if statement was actually incorrect.

[alangpierce]

I think the shouldElideExportedIdentifier check should actually be kept with the previous if statement, so that implicitly-elided exports are skipped as well. Otherwise, the module.exports = exports.default; line will be incorrectly added for this code:

type T = number;
export {T as default};

[sxzz]

If we kept it, the unit test will fail, because the shouldElideExportedIdentifier returning value of export { x as default } from './foo' is true.

---

In your case, we can actually export both a value and a type as default at the same time (though with TS errors)

https://www.typescriptlang.org/play?#code/PTAEAEBcGcFoDsD2BjAFgU2QawFA8gJ4AO6oAKqALyjwCuAtgEboBOA3DugB5GIuSgA3uVABDaKAAm6AGajaAGwEBfDpx58B0uYoEBGIA

But the output will remain only one export default and no named export, so we can add module.exports = exports.default.

[alangpierce]

If we kept it, the unit test will fail

Good point, sorry for not trying that out. I think my case above is still valid, though, see babel example. If the export is erased due to being a type, it shouldn't count when deciding whether or not to emit module.exports = exports.default.

This is actually very similar to #806 that I just implemented, but that fix was just for ESM output since CJS happened to already be working. The core issue, I think, is that export {...} and export {...} from 'foo' are handled by the same code path, but only the first one should erase type-only exported names. This didn't actually cause any correctness issues for CJS in normal export elision, but it does matter for this case of implementing enableLegacyBabel5ModuleInterop. Just like in #806, I think the fix is to look ahead to distinguish the two cases.

This PR certainly puts the feature in better shape, so let me merge it now, and I'll do a quick follow-up to refactor the code from #806 to handle the type case here I mentioned. Thanks again!

we can actually export both a value and a type as default at the same time

Agreed that this is possible, but I think if default is only a type, then it should be ignored for the purposes of deciding whether to emit module.exports = exports.default. That's consistent with Babel's behavior linked above.

[alangpierce]

Also, adding a little context here for myself and future readers, since I was needing to remind myself how add-module-exports works:

When using enableLegacyBabel5ModuleInterop, analogous to babel-plugin-add-module-exports, the expected behavior is to add module.exports = exports.default; (making the whole CJS module object the default export) when there is a default export but no named exports. My original implementation missed some cases, which are fixed by this PR:

• Type-only exports should not count as a named export for these purposes. Fixed by rearranging some cases so hadNamedExport isn't set to true, and by considering each export individually in export {... lines.
• Explicitly exporting the name default should count as a default export, not a named export, which is fixed by doing a string match on the exported name.

[github-actions]

Benchmark results

Before this PR: 321 thousand lines per second
After this PR: 322.9 thousand lines per second

Measured change: 0.6% faster (0.75% slower to 1.5% faster)
Summary: Likely no significant difference