Fix naming collisions between named exports and wildcard exports

[astegmaier]

↪️ Pull Request

This fixes #8331.

💻 Examples

Previously, when a name contained in a module exported through a wildcard export conflicted with another named export, we would sometimes generate incorrect .d.ts bundles that included the wildcard export name instead of the named export. For example:

src/index.ts

export const nameConflict = { messageFromIndex: "this instance of nameConflict is from index.ts" };
export * from "./other"; // Note: this comes _after_ the top-level export above.

src/other.ts

export const nameConflict = { messageFromOther: "this instance of nameConflict is from othert.ts" };

...would produce...

types.d.ts

export const nameConflict: {
    messageFromOther: string;
};
declare const _nameConflict1: {
    messageFromIndex: string;
};

...instead of...

export const nameConflict: {
    messageFromIndex: string;
};

With this change, we do the right thing, no matter the order of the exports.

I tested both parcel and tsc runtime behavior and in such a situation, the higher-level named export always "wins" - i.e. the nameConflict in other.ts is completely inaccessible, so it can be shaken.

I also verified that we will still do the right thing in cases where the lower-level name export is referenced by some other export. For example, if you add this file to the above program

src/name-conflict.ts

import { nameConflict } from "./other1";
export const consumer: typeof nameConflict = { messageFromOther1: "This variable uses the type of the nameConflict1 variable defined in other1.ts.." };

...and export it at the top level...

src/index.ts

export const nameConflict = { messageFromIndex: "this instance of nameConflict is from index.ts" };
export * from "./other";
export { consumer } from "./consumer"; // <--this was added

...then your types.d.ts bundle will be...

declare const _nameConflict1: {
    messageFromOther1: string;
};
export const consumer: typeof _nameConflict1;
export const nameConflict: {
    messageFromIndex: string;
};

...which is correct.

Both of these cases are reflected in the unit test I added.

One other case that I considered, but where we already do the right thing, is where two wildcard exports point to modules that have a name conflict within them. For example, if your program is...

index.ts

export * from "./other1"
export * from "./other2"

other1.ts

export const nameConflict = { messageFromOther1: "this instance of nameConflict is from other1.ts" };

other2.ts

export const nameConflict = { messageFromOther2: "this instance of nameConflict is from other2.ts" };

... we will bubble up the following warning from tsc at compile-time:

    1 | export * from "./other1.ts";
  > 2 | export * from "./other2.ts";
  >   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Module "./other1" has already exported a member named 'nameConflict'. Consider explicitly re-exporting to resolve the ambiguity.
    3 |

🚨 Test instructions

The unit test should be sufficient, but in case you want to play around with other use-cases end-to-end (and test how the resulting d.ts files would be consumed by other packages), you can use this test repo combined with yarn link.

✔️ PR Todo

• Added/updated unit tests for this change
• Filled out test instructions (In case there aren't any unit tests)
• Included links to related issues/PRs

[devongovett]

This is an interesting case where Parcel's behavior doesn't match TSC's I think. When compiling JS, the last one wins. But looks like TSC prioritizes named exports over export *? Example

Might be bad if the compiled types don't match the code?

cc. @mischnic

[astegmaier]

Might be bad if the compiled types don't match the code?

Totally agree. But I think in this case, the current type definitions from parcel do not match the runtime behavior, and this PR changes it to align (see below).

When compiling JS, the last one wins.

This is not what I'm seeing in my tests. The runtime behavior of both Parcel and Tsc seems to be "named exports always win over wildcard exports". Details are below - let me know if you think I might be overlooking something.

I have a test monorepo with two libraries that have the same source code, but one is built with tsc, and the other with parcel. (you can run it yourself here):

index.ts

// Case 1: the top-level export is _after_ the wildcard export.

export * from "./other1";
export const nameConflict1 = { messageFromIndex: "this instance of nameConflict1 is from index.ts" };

// Case 2: the top-level export is _before_ the wildcard export <-- this will give you incorrect d.ts in parcel

export const nameConflict2 = { messageFromIndex: "this instance of nameConflict2 is from index.ts" };
export * from "./other2";

other1.ts

export const nameConflict1 = { messageFromOther2: "this instance of nameConflict1 is from other1.ts" };

other2.ts

export const nameConflict2 = { messageFromOther2: "this instance of nameConflict2 is from other2.ts" };

I consume and test the libraries like this (as written, this test passes).

import * as Parcel from "test-library-parcel";
import * as Tsc from "test-library-tsc";

describe("Wildcard export names conflicting with named exports", () => {
    it("Tsc should prefer the named export when it is _after_ the wildcard export.", () => {
        expect(Tsc.nameConflict1.messageFromIndex).toBe("this instance of nameConflict1 is from index.ts");
    });
    it("Parcel should prefer the named export when it is _after_ the wildcard export.", () => {
        expect(Parcel.nameConflict1.messageFromIndex).toBe("this instance of nameConflict1 is from index.ts");
    });
    it("Tsc should prefer the named export when it is _before_ the wildcard export.", () => {
        expect(Tsc.nameConflict2.messageFromIndex).toBe("this instance of nameConflict2 is from index.ts");
    })

    // This is the bug:
    it("Parcel should prefer the named export when it is _before_ the wildcard export, but the type definitions don't align with this behavior.", () => {
        // @ts-expect-error: Property 'messageFromIndex' does not exist on type '{ messageFromOther2: string; }
        expect(Parcel.nameConflict2.messageFromIndex).toBe("this instance of nameConflict2 is from index.ts");
        // The d.ts file generated by parcel says "messageFromOther2" is a "string", but it is undefined at runtime.
        expect(Parcel.nameConflict2.messageFromOther2).toBe(undefined);
    });
})

When I debug the runtime bundle generated by parcel, I see the calls to $parcel$exportWildcard() appear at the bottom of the bundle -- after the named exports are defined. When those functions executed, they no-op because they discover that the destination object already has a property called nameConflictX:

function $parcel$exportWildcard(dest, source) {
  Object.keys(source).forEach(function(key) {
    if (key === 'default' || key === '__esModule' || dest.hasOwnProperty(key)) {
     // We return early here in this case because dest.hasOwnProperty("nameConflictX") is true.
      return;
    }
    // Rest of the function
  });

I was assuming this is correct because it matches tsc. (Although perhaps both are wrong? Not sure where in the spec defines this behavior...)

[astegmaier]

@devongovett - I noticed that I overlooked the link to the example in the REPL that you posted .

Unfortunately, when I click on it, it seems broken - here's what I see:

So I put the same code in a test repo and ran it, and here's what I see:

The value 456 comes from the named export in file1.js, not the wildcard-exported file (other.js)

Changing the order of the exports in file1.js has no effect - it will still print 456. The rule seems to be "named exports win over wildcard exports", rather than "last export wins". This is consistent with the test results I ran above.

[mischnic]

Huh, I'll have to try it with Edge as some point, works on Chrome macOS:

I haven't had time to look at this in detail yet, but:

Although perhaps both are wrong? Not sure where in the spec defines this behavior...

A good rule of thumb is: assume v8 implements the spec correctly, which also shows that named exports have a higher precedence than export stars

[astegmaier]

A good rule of thumb is: assume v8 implements the spec correctly, which also shows that named exports have a higher precedence than export stars

Makes sense. So it seems that at least the goals of this PR - to make sure we respect that rule in the generated .d.ts are correct. Is there anything I can do to improve the implementation?

[ivanbanov]

@devongovett is it expected to have this PR merged any time soon? :)