Replaces the default module index resolver with '/index' instead of '' when handling internal routing for dts bundles

[orta]

Fixes #31280

Previously when bundled:

// @Filename: index.ts
export * from "./nested";

// @Filename: nested/base.ts
import { B } from "./shared";

export function f() {
    return new B();
}

// @Filename: nested/derived.ts
import { f } from "./base";

export function g() {
    return f();
}

// @Filename: nested/index.ts
export * from "./base";
export * from "./derived";
export * from "./shared";

// @Filename: nested/shared.ts
export class B {}

Would convert to:

declare module "nested/shared" {
    export class B {
    }
}
declare module "nested/base" {
    import { B } from "nested/shared";
    export function f(): B;
}
declare module "nested/derived" {
    export function g(): import("nested").B;
//                               ^^^^^^^
}
declare module "nested/index" {
    export * from "nested/base";
    export * from "nested/derived";
    export * from "nested/shared";
}
declare module "index" {
//              ^^^
    export * from "nested/index";
}

Which is kinda weird:

declare module "index" {
    export * from "nested/index";
}

Now, it would raise a compiler error:

error TS1386: The bundledPackageName option must be provided when using outFile and node module resolution with declaration emit.

Which requires you to add a new tsconfig option for the name of your package:

declare module "my-pkg/nested/shared" {
     export class B {
     }
 }
 declare module "my-pkg/nested/base" {
     import { B } from "my-pkg/nested/shared";
     export function f(): B;
 }
 declare module "my-pkg/nested/derived" {
     export function g(): import("my-pkg").B;
 }
 declare module "my-pkg/nested" {
     export * from "my-pkg/nested/base";
     export * from "my-pkg/nested/derived";
     export * from "my-pkg/nested/shared";
 }
 declare module "my-pkg" {
     export * from "my-pkg/nested";
 }

Old PR

Would have turning into:

But now that type import looks like:

    export function g(): import("index").B;

Which actually exists.

[sandersn]

The new test looks good and it doesn't break any old tests.

However, you should probably also get signoff from @rbuckton or @weswigham since they understand emit issues better than I do.

[orta]

Rough gist: the module names are wrong, not the import

Chatted with @weswigham - when you have commonjs set as the target, you basically never want a .d.ts which would look like this - the core issue is in the declared module names (nothing in the generated .d.ts here is portable) without either:

• requiring code like /// <amd-module-name name="thing" /> in some/all files
• knowing the package for the package name and adding it to the module names:

declare module "packagename/nested/shared" {
    export class B {
    }
}
declare module "packagename/nested/base" {
    import { B } from "packagename/nested/shared";
    export function f(): B;
}
declare module "packagename/nested/derived" {
    export function g(): import("packagename/nested").B;
}
declare module "packagename/nested" {
    export * from "packagename/nested/base";
    export * from "packagename/nested/derived";
    export * from "packagename/nested/shared";
}
declare module "packagename" {
    export * from "packagename/nested";
}

Which could come from a compiler option. (an implicit look up wouldn't work in all cases) - Wes feels this is worth it.

[sandersn]

Again, looks basically good, but I'd like to see the output of the test that's missing baselines, and I have a follow-up question for @weswigham 's suggestion.

[sheetalkamat]

I dont think adding new option is right thing to do here.
If i read the original issue, the chosen module name isnt correct. It is either chosing incorrect file to import or transformation of name of that importing file to what it means in d.ts is incorrect . And i think fixing that is the right thing to do.

[weswigham]

@sheetalkamat declarations with outFile and module: commonjs was never meant to work, because it was originally an error. Only amd (and classic resolution) bundles were really considered. And even then, we questioned if the module names made sense and users said "nah, we'll just fix it up with a script later". When emitDeclarationsOnly was added, we did not consider what missing information was required to create a functioning declaration bundle. The new option feeds in that information.

The chosen module name is not incorrect. It is applying node module resolution, which is what was selected by the user. What wasn't happening was generating module names that actually make sense under node module resolution because, again, until the emitDeclarationsOnly flag was added, the cjs + outfile + declarations combination was an error. The only other way to get same module names would be to require // <amd-module name="whatever" /> directives in every file, which is clearly more tedious and error prone.

[weswigham]

Like, nobody actually wants their root module named index - the declaration file clearly isn't portable that way, as it'd conflict with any other project running the same setup. Same with src folders and everything else. You need a module name to key off of.

[orta]

Now that I've got this updated, and the are in baselines too, I feel like I want to dig into some of these baselines to make sure we're not over-reporting on this.

[orta]

Alright, I've updated this PR and tests are green - I'm a little unsure how to get the final part passed because I can't add "bundledPackageName": "shims" to that tsconfig without updating the LKG? Any clues?

[sandersn]

@sheetalkamat @weswigham can you take another look and decide whether the updated PR is good? (I've somewhat arbitrarily assigned the PR to @sheetalkamat because I want to have a single assignee).

[weswigham]

I helped design/write this one, so I'm biased anyway~ (and think this is pretty good)

[orta]

OK, I've made changes to the tsconfigs in this repo to negate the error instead 👍🏻

[DanielRosenwasser]

I think this ended up being is a breaking change for people who were using post-processors to make non-module code work with modules (like our team).

var _chai: typeof import("chai") = require("chai")

[andrewbranch]

I'm late to the party here, but I don't think it's right to pass the bundled package reference to tryGetModuleNameFromPaths. These values get tested for equality with the values in the paths value arrays (with * substitutions applied), which are absolute paths. bundledPkgReference will presumably not be an absolute path, so this will prevent paths from ever being used to generate a module specifier in tandem with bundledPackageName. Or, maybe that's the intent? I can't totally wrap my mind around how/why these options would ever be used together.

[DanielRosenwasser]

Coming back to this issue, I really want to revisit @sheetalkamat's concern here. I don't think I would have anticipated this option nor this break would go in without some design meeting discussion. I also feel uneasy about the naming - why would we call this bundledPackageName if it only affects .d.ts emit but not .js emit?

[weswigham]

It actually does affect .js emit - just only amd-style .js emit, because that's the only format where the package name appears in the generated code. (Previously it was controllable only with a /// <amd-module name="whatever"/> directive)