Bugfix allow re-export of existing named members

[cpitt]

Add configurable: true to Object.defineProperty to allow overwriting existing named exports.

Consider the following valid code

// lib1.ts
export const a = '1';

// lib2.ts
export const a = '2'

// index.ts
export * from 'lib1.ts'
export * from 'lib2.ts'
export { a } from 'lib2.ts'

Currently tsc and babel compile this fine but sucrase fails with

TypeError: Cannot redefine property: a
        at Function.defineProperty (<anonymous>)

[codecov]

Codecov Report

Merging #698 (dc899cf) into main (2658878) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #698   +/-   ##
=======================================
  Coverage   82.73%   82.73%           
=======================================
  Files          56       56           
  Lines        5694     5694           
  Branches     1343     1343           
=======================================
  Hits         4711     4711           
  Misses        700      700           
  Partials      283      283           

Impacted Files	Coverage Δ
src/HelperManager.ts	100.00% <ø> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[github-actions]

Benchmark results

Before this PR: 254.3 thousand lines per second
After this PR: 262.4 thousand lines per second

Measured change: 3.19% faster (0.85% faster to 4.57% faster)
Summary: Probably faster

[alangpierce]

Hi @cpitt, thanks! I'm a bit busy right now but should be able to take a look in the next few days.

[alangpierce]

Thanks! And sorry for the delay.

This does move some cases a little away from true spec compliance by making the property configurable, though I'd be very surprised if that caused trouble. Generally, Sucrase is ok with being more permissive than other transpilers/implementations, though usually that comes up in terms of compile-time errors that Sucrase skips rather than runtime behavior differences.

There's actually a way to write a test for cases like these using assertMultiFileOutput in imports-test.ts, so I'll add a quick test for this case in a follow-up and publish.

As part of investigating this, I took a look at the behavior with various situations and different tools:

	Your example	Just two star exports	Named export at beginning
Sucrase before	Crashes, Cannot redefine property: a	Exports 1, not configurable	Exports 2, not configurable
Sucrase after	Exports 2, configurable	Exports 1, configurable	Exports 2, configurable
Babel	Exports 2, not configurable	Crashes, Cannot redefine property: a	Exports 2, not configurable
TypeScript	Exports 2, configurable	Exports 1, not configurable	Exports 2, configurable
Node.js 18 ESM	Exports 2, not configurable	Crashes, The requested module './index.js' contains conflicting star exports for name 'a'	Exports 2, not configurable

Some notes from the comparison here and some additional testing:

• It looks like the ideal behavior (assuming node implements this correctly) is that ambiguous star re-exports are disallowed, but if an explicit re-export clears up the ambiguity, then that's fine. So your example code is perfectly valid and should export '2'.
• Really, all exports are supposed to be non-configurable in ESM, whether it's a star re-export, a named re-export, or a direct export. Node gets this correct, of course, but all transpilers use a configurable property for direct exports, and TS uses a configurable property for named re-exports. So there's certainly precedent for configurable properties, and really in the vast majority of cases (i.e. direct exports) they are configurable in all transpilers.

To be clear, here's how this case works in Babel and TS:

• Babel creates a _exportNames object that's hoisted near the top of the file and includes the names of all explicitly-exported values. The export * from syntax knows to skip these.
• TypeScript injects a line exports.a = void 0; before any import code, which sets a as a configurable property, and later defineProperty calls keep it configurable. Explicit re-exports take precedence over star re-exports, regardless of ordering, because star re-exports don't modify the exports object if the property already exists.

Both of these approaches are certainly doable in Sucrase (and the babel one seems better if I were to pick), though I think the simpler approach in this PR is the right approach for now.

[alangpierce]

Just released as sucrase 3.21.1.

[cpitt]

Thanks for the detailed response @alangpierce I really appreciate it. I learned a lot from that post.