Skip to content

Declaration emit can't utilize exports map information and ends up reporting "this is likely not portable" #56107

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

Closed
Andarist opened this issue Oct 14, 2023 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@Andarist
Copy link
Contributor

🔎 Search Terms

declaration emit nodenext exports map package.json inferred type annotation necessary

🕗 Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

No response

💻 Code

// @strict: true
// @declaration: true
// @module: nodenext
// @noImplicitReferences: true

// @filename: node_modules/effect/dist/declarations/src/Data.d.ts
import type * as Types from "./Types.js";
import type * as Equal from "./Equal.js";

export type Data<A extends Readonly<Record<string, any>> | ReadonlyArray<any>> =
  Readonly<A> & Equal.Equal;

export declare const TaggedClass: <Key extends string>(
  tag: Key,
) => new <A extends Record<string, any>>(
  args: Types.Equals<Omit<A, keyof Equal.Equal>, {}> extends true
    ? void
    : Omit<A, keyof Equal.Equal>,
) => Data<
  A & {
    _tag: Key;
  }
>;

// @filename: node_modules/effect/dist/declarations/src/Equal.d.ts
import * as Hash from "./Hash.js";

export declare const symbol: unique symbol;

export interface Equal extends Hash.Hash {
  [symbol](that: Equal): boolean;
}

// @filename: node_modules/effect/dist/declarations/src/Hash.d.ts
export declare const symbol: unique symbol;

export interface Hash {
  [symbol](): number;
}

// @filename: node_modules/effect/dist/declarations/src/Types.d.ts
export type Equals<X, Y> = (<T>() => T extends X ? 1 : 2) extends <
  T,
>() => T extends Y ? 1 : 2
  ? true
  : false;

// @filename: node_modules/effect/dist/declarations/src/index.d.ts
export * as Data from "./Data.js";

// @filename: node_modules/effect/dist/effect.cjs.d.ts
export * from "./declarations/src/index";

// @filename: node_modules/effect/Equal/dist/effect-Equal.cjs.d.ts
export * from "../../dist/declarations/src/Equal";

// @filename: node_modules/effect/Types/dist/effect-Types.cjs.d.ts
export * from "../../dist/declarations/src/Types";

// @filename: node_modules/effect/package.json
{
  "name": "effect",
  "exports": {
    ".": "./dist/effect.cjs.js",
    "./Equal": "./Equal/dist/effect-Equal.cjs.js",
    "./Types": "./Types/dist/effect-Types.cjs.js"
  }
}

// @filename: src/index.ts
import { Data } from "effect";
export class Foo extends Data.TaggedClass("Foo")<{}> {}

🙁 Actual behavior

==== src/index.ts (2 errors) ====
    import { Data } from "effect";
    export class Foo extends Data.TaggedClass("Foo")<{}> {}
                 ~~~
!!! error TS2742: The inferred type of 'Foo' cannot be named without a reference to '../node_modules/effect/dist/declarations/src/Equal'. This is likely not portable. A type annotation is necessary.
                 ~~~
!!! error TS2742: The inferred type of 'Foo' cannot be named without a reference to '../node_modules/effect/dist/declarations/src/Types'. This is likely not portable. A type annotation is necessary.

🙂 Expected behavior

//// [index.d.ts]
import { Data } from "effect";
declare const Foo_base: new <A extends Record<string, any>>(args: import("effect/Types").Equals<Omit<A, keyof import("effect/Equal").Equal>, {}> extends true ? void : Omit<A, keyof import("effect/Equal").Equal>) => Data.Data<A & {
    _tag: "Foo";
}>;
export declare class Foo extends Foo_base<{}> {
}
export {};

Additional information about the issue

I think that it would be best if the listed exports entries (especially the ones not relying on wildcards but the wildcard support would also be nice) could be automatically loaded by the compiler at some stage of the declaration emit.

In this case, this would allow the compiler to discover effect/Types and effect/Equal and getAlternativeContainingModules could return them.

cc @andrewbranch as package.json#exports is your turf 😉

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Oct 16, 2023
@andrewbranch andrewbranch modified the milestone: TypeScript 5.3.1 Oct 30, 2023
@andrewbranch
Copy link
Member

The compiler architecture doesn’t allow for loading more files after program construction. We’d have to include all exports entrypoints from all packages up front just in case we need them, which seems like an unacceptable performance tradeoff. This is subtle but I think the structure of this package is inherently problematic for TS—you should try to avoid a situation where you have something exported from a public entrypoint whose signature depends on private modules, and here “private” includes “only reachable through re-exporting modules that TS had no reason to parse.” I would possibly recommend using a triple-slash reference in dist/declarations/src/Equal.d.ts (the private module) pointing to dist/effect-Equal.cjs.d.ts (the public entrypoint) to ensure whenever TS loads the private module, it will also load the only module it can use to write a module specifier that gives access to the exports within.

@andrewbranch andrewbranch added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Needs Investigation This issue needs a team member to investigate its status. labels Oct 30, 2023
@andrewbranch andrewbranch removed their assignment Oct 30, 2023
@andrewbranch
Copy link
Member

On closer inspection, I think this is not the same as the issue in #56174

@Andarist
Copy link
Contributor Author

This is subtle but I think the structure of this package is inherently problematic for TS—you should try to avoid a situation where you have something exported from a public entrypoint whose signature depends on private modules

This is quite hard to manage though. With inference on it's hard to know what we depend on and if any private modules are used. In this case, the declaration emit of the package itself was done just fine - the problem raised in the consuming project. I see how this might be hard to fix with the mentioned design limitation in mind but, damn, we lack some early errors
that would help us to recognize the problem sooner 😢

I would possibly recommend using a triple-slash reference in dist/declarations/src/Equal.d.ts (the private module) pointing to dist/effect-Equal.cjs.d.ts (the public entrypoint) to ensure whenever TS loads the private module, it will also load the only module it can use to write a module specifier that gives access to the exports within.

This sounds like something we could do. I wonder - should it be OK to reference an ESM path from a CJS declaration file?

@andrewbranch
Copy link
Member

andrewbranch commented Oct 30, 2023

You need a resolution-mode attribute, which unfortunately will be an error outside of node16/nodenext before 5.3. You can ts-ignore it 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants