Skip to content
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

declaration emit uses absolute path when inferring type #54743

Closed
eps1lon opened this issue Jun 22, 2023 · 6 comments
Closed

declaration emit uses absolute path when inferring type #54743

eps1lon opened this issue Jun 22, 2023 · 6 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@eps1lon
Copy link
Contributor

eps1lon commented Jun 22, 2023

Bug Report

🔎 Search Terms

emit declaration path
Similar to #38111 but #38111 uses the same source for import whereas the repro here uses different import sources.

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________

⏯ Playground Link

Repro: https://github.com/eps1lon/ts-proj-references-abs-path-repro
Spans multiple files which is not suitable for Playground

💻 Code

useDialogNavigation.ts (emits expected package identifier import)

declare function useCallback<T extends Function>(callback: T): T;

import { navigateBack, close as _close } from "@klapp/navigation";

export const useDialogNavigation = () => {
  const close = useCallback(() => {
    return _close(null as any);
  });

  const back = useCallback(() => {
    return navigateBack(null as any);
  });

// expected
// back: () => import("@klapp/navigation").MightReturnComponentId
  return { back, close };
};

// useNavigationEvents.ts (emits unexpected path import)

import { useDialogNavigation } from "./useDialogNavigation";
export const useNavigationEvents = () => {
  const { back, close } = useDialogNavigation();

  return {
// unexpected
// back: () => import("packages/navigation").MightReturnComponentId
    back,
    close,
  };
};

// tsconfig.json

{
  "extends": "../../tsconfig.base.json",
  "include": ["./**/*"]
}

// tsconfig.base.json

{
  "compilerOptions": {
    "baseUrl": "./",
    "declaration": true,
    "moduleResolution": "NodeNext",
    "noEmit": false,
    "outDir": "./types/dist",
    "rootDir": ".",
    "strict": true
  }
}

🙁 Actual behavior

One file uses import("@klapp/navigation") while another uses import("packages/navigation")

🙂 Expected behavior

Both use import("@klapp/navigation")

@RyanCavanaugh
Copy link
Member

Both packages/navigation and @klapp/navigation are legal ways to refer to that module, and declaration emit has no way to know which you wanted.

I'd recommend unsetting baseUrl in your tsconfig; this will make the first one illegal and cause the second to be emitted. Barring that, you'd need to have a manual import in that file to tell the emitter which you wanted.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Jun 22, 2023
@Andarist
Copy link
Contributor

From baseUrl docs:

This resolution has higher priority than lookups from node_modules.
This feature was designed for use in conjunction with AMD module loaders in the browser, and is not recommended in any other context.

I can only second that. I had multiple terrible experiences with baseUrl in the past when somebody set this in tsconfig, likely just copy-pasting from another config. Perhaps it would be even best to add a prominent warning to the docs about this being a (sort of) legacy-ish feature?

@eps1lon
Copy link
Contributor Author

eps1lon commented Jun 22, 2023

baseUrl used to be required to work with paths pre 4.1 which is why we (and probably most) end up still having it.

It's also pretty easy to fall into the trap of adding baseUrl given the current error message when paths don't contain the leading ./: "Non-relative paths are not allowed when 'baseUrl' is not set. Did you forget a leading './'?". It is not that surprising that people opt for the DRY baseUrl instead of repeating the leading ./.

Both packages/navigation and @klapp/navigation are legal ways to refer to that module, and declaration emit has no way to know which you wanted.

Then why does TypeScript then complain that it can't find the module at packages/navigation we it would use the mitted declaration in a composite project? It wouldn't be a problem per se since we don't ship these types. But the declarations emitted are used for incremental builds where TS specifically says "can't find module 'packages/navigation'". So I don't know what you mean with "legal way" because TypeScript itself cannot handle this emit using the same tsconfig. Which begs the question: Why not error when this option is set in conjunction with node-ish resolutions? What valid use case would there be for the tsconfig I shared?

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Jun 23, 2023

Then why does TypeScript then complain...

If you take Project A under settings X, produce a .d.ts file, and try to consume it from Project B under settings Y, then it's entirely possible that differences between X and Y (or even file layout changes) mean that a module specifier that works in A doesn't work in B. packages/navigation works in this compilation so it's considered a legal path; knowing whether or not your downstream projects are configured in a way to also accept that path would require precognition.

@eps1lon
Copy link
Contributor Author

eps1lon commented Jun 23, 2023

It was emitted and consumed by the same project that used incremental builds. I reduced it to just highlight the incorrect emission.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2023
eps1lon added a commit to eps1lon/next.js that referenced this issue Apr 5, 2024
`baseUrl` is not supposed to be used for catch-all resolutions.
It's not recommended outside of AMD modules: https://www.typescriptlang.org/tsconfig#baseUrl

microsoft/TypeScript#54743 has some more context.
eps1lon added a commit to eps1lon/next.js that referenced this issue Apr 8, 2024
`baseUrl` is not supposed to be used for catch-all resolutions.
It's not recommended outside of AMD modules: https://www.typescriptlang.org/tsconfig#baseUrl

microsoft/TypeScript#54743 has some more context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

4 participants