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

Import types in d.ts files contain relative paths to node_modules #24599

Closed
jwbay opened this issue Jun 1, 2018 · 31 comments · Fixed by #25110
Closed

Import types in d.ts files contain relative paths to node_modules #24599

jwbay opened this issue Jun 1, 2018 · 31 comments · Fixed by #25110
Assignees
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files Fixed A PR has been merged for this issue

Comments

@jwbay
Copy link
Contributor

jwbay commented Jun 1, 2018

TypeScript Version: 3.0.0-dev.20180601

Search Terms: declaration import types

Code
src/one/two/export.ts:

import * as CSS from 'csstype';
export const use = (f: CSS.Properties['fontWeight']) => f;

src/one/two/consumer.ts:

import { use } from "./export";
export const asdf = use ;

Expected behavior:
consumer.d.ts has something like

export declare const asdf: (f: import("csstype/index").FontWeightProperty) => import("csstype/index").FontWeightProperty;

Actual behavior:
It has relative paths to node_modules, based on the source file's location at compile time. This places an assumption on the location of the d.ts file when it's consumed externally.

export declare const asdf: (f: import("../../../node_modules/csstype/index").FontWeightProperty) => import("../../../node_modules/csstype/index").FontWeightProperty;

Playground Link: N/A

Related Issues: #24556, but csstype does specify a types key in package.json

@weswigham
Copy link
Member

Small repro:

// @declaration: true
// @filename: node_modules/foo/index.d.ts
export interface SomeProps {
    x: number;
}

export function foo(): SomeProps;
// @filename: entry.ts
import { foo } from "foo";
export const x = foo();

emits

export declare const x: import("./node_modules/foo/index").SomeProps;

you'd much rather it use the node module resolution style "foo" than "./node_modules/foo/index" 😛

There is a wrench, though:

// @declaration: true
// @filename: node_modules/foo/other/index.d.ts
export interface OtherIndexProps {}
// @filename: node_modules/foo/other.d.ts
export interface OtherProps {}
// @filename: node_modules/foo/index.d.ts
import { OtherProps } from "./other";
import { OtherIndexProps } from "./other/index";

export function foo(): [OtherProps, OtherIndexProps];
// @filename: entry.ts
import { foo } from "foo";
export const x = foo();

in this case, writing import("foo/other"), by nodejs rules, always yields the loose file. So you need to know that you can't just write import("foo/other") to refer to import("foo/other/index"), since the loose file overrides it. All this means that to do this right, you pretty much need to check the full fs to ensure you don't shorten to something that refers to something else.

Minimally, stripping off the leading relative dir parts and node_modules part is safe, reduces verbosity, and improves portability, though.

@mhegazy mhegazy added Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files labels Jun 3, 2018
@zsgalusz
Copy link

zsgalusz commented Jun 7, 2018

I can reproduce this issue in 2.9.1, it's a major blocker for us in upgrading to TS2.9.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jun 7, 2018
unional added a commit to unional/tersify that referenced this issue Jun 8, 2018
unional added a commit to unional/satisfier that referenced this issue Jun 8, 2018
unional added a commit to unional/assertron that referenced this issue Jun 8, 2018
@esamattis
Copy link

esamattis commented Jun 14, 2018

TypeScript 2.9.2 made some changes to this but it's still broken.

2.9.1 generated type import paths like ./node_modules/redux/index but now in 2.9.2 they are ../../../../../Users/esamatti/code/mylibrary/node_modules/redux

In my case just redux would be the desired output.

esamattis added a commit to esamattis/redux-stack that referenced this issue Jun 14, 2018
Workaround for microsoft/TypeScript#24599

Manually fixed the type imports in the emitted declaration files
@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2018

@epeli can you give typescript@next a try?

@weswigham
Copy link
Member

@mhegazy we still haven't published a nightly this week.

@esamattis
Copy link

Sure. Can you ping me when it's out so I can test it?

@osdiab
Copy link

osdiab commented Jun 16, 2018

I too am seeing this problem; my imports in the declarations are going all the way to my filesystem root. I think this might have something to do with the baseUrl config, pretty sure it shouldn't apply here because this is a relative path but it seems to impact what gets outputted (unless I'm misunderstanding the use case of baseUrl).

There's a nested dependency in my code, where I have this import:

// File: src/routes/listOperations.ts
import { createApiRoute } from "../";
export const listOperations() = createApiRoute<ListOperationsEndpoint>( //... );

// File: src/routes/index.ts"
import { ApiContext } from "../app";
export const createApiRoute<Endpoint extends ApiEndpoint> = (ctx: ApiContext<ApiEndpoint["authenticated"]>): Promise<void> => //...

// File src/app.ts
export type AuthenticatedContext = // ...
export type UnauthenticatedContext = // ...
export type ApiContext<Authenticated extends boolean> = Authenticated ? AuthenticatedContext : UnauthenticatedContext

If baseUrl is not present, then I get this output:

// File: build/routes/listOperations.d.ts
export declare const listOperations: (operations: CollectionReference) => (ctx: import("../../../../../../../../Users/omar/code/raha/raha-api/src/app").UnauthenticatedContext) => Promise<void>;

Not what I expect! I would expect it to be ../../app.

Then, if I set baseUrl: "./" in tsconfig.json I get:

export declare const listOperations: (operations: CollectionReference) => (ctx: import("src/app").UnauthenticatedContext) => Promise<void>;

Yeah, shouldn't be an absolute path now, since I have no paths entry in tsconfig. Pretty sure baseUrl shouldn't be affecting this since this is not an absolute path...

@weswigham
Copy link
Member

@osdiab do you see the same thing with typescript@next?

@osdiab
Copy link

osdiab commented Jun 16, 2018 via email

@weswigham
Copy link
Member

What're all the rest of your tsconfig options (outDir, rootDir, etc), and are there any symlinks in your project?

@osdiab
Copy link

osdiab commented Jun 17, 2018 via email

@esamattis
Copy link

esamattis commented Jun 18, 2018

If you need something to reproduce this with you can use my redux-stack lib. There is no baseUrl used in this project.

git clone https://github.com/epeli/redux-stack.git
cd redux-stack
git checkout 21879cfb8bb35a6fed4fcb62cf3cd3d2eaf1935a # current master at the time of writing
npm install
npm install typescript@next # get 3.0 dev version
./node_modules/.bin/tsc

and checkout the type imports in typings/src/configure-store.d.ts. With 3.0.0-dev.20180616 they are like ../../../../../Users/esamatti/code/redux-stack/node_modules/redux.

@abettadapur
Copy link

abettadapur commented Jun 18, 2018

I am also seeing this issue.

//foo.d.ts
export declare const areTeamsLoading: import("../../../../../../../../../../npm/default/node_modules/reselect/src/index").OutputSelector<ITeamAwareState, boolean, (res: ITeamState) => boolean>;

We have a custom build process that copies the build files to a slightly different directory structure than the source code, so this relative path is incorrect.

@jods4
Copy link

jods4 commented Jun 19, 2018

Got biten by this.

I have a function whose return type is implicit and lives in another file my project.

When compiling with -d I get following output:

Typescript 2.9.1:

readonly scope: import("./scope").Scope;

Typescript 2.9.2:

readonly scope: import("../../../../../../Code/x/packages/lib/src/templating/scope").Scope;

This is a very bad regression!

2.9.1 worked well enough, because it stayed inside my declarations folder.
2.9.2 pulls in the src folder, which brings all my .ts lib sources into consuming projects and everything breaks hard.

@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label Jun 19, 2018
@tyler-johnson
Copy link

Hello, I am bit confused as this issue seems to be tracking two different problems.

  1. paths to node_modules are showing up as relative paths ./node_modules/package instead of just package
  2. paths are going back to root, ../../../Users/user/package/file instead of just ./file

So does #25110 solve both or just the 2nd issue?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 29, 2018

@tyler-johnson please give typescript@next a try tonight, and if you are still seeing issues please file a new ticket with more details.

@nyrkovalex
Copy link

@mhegazy I can confirm excessive ../../../ path generation is fixed in typescript@3.0.0-dev.20180630.

@goloveychuk
Copy link

I still see

export declare const fetchFilterCategories: ({ entityType, entityId }: any) => import("node_modules/axios").AxiosPromise<any>;

to reproduce:

import Axios from 'axios'

export const fetchFilterCategories = ({ entityType, entityId }: any) => {
    return Axios.get(`sd`);
};

Also have such references

/// <reference path="../node_modules/@types/lodash/common/common.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/array.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/collection.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/date.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/function.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/lang.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/math.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/number.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/object.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/seq.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/string.d.ts" />
/// <reference path="../node_modules/@types/lodash/common/util.d.ts" />

compile to reproduce:

import {mapValues} from 'lodash';
export const mapDispatchToProps = mapValues

ts version: Version 3.0.0-dev.20180630

@ghost
Copy link

ghost commented Jul 3, 2018

Should be fixed by #25364, please open a new issue if you see any ../../node_modules-like paths in typescript@next.

@weswigham
Copy link
Member

typescript@next tomorrow for anyone watching the thread.

@esamattis
Copy link

3.0.0-dev.20180705 seems to have fixed this, thank you!

@OliverJAsh
Copy link
Contributor

Is there any chance of getting this out into a 2.9 patch release, so we don't have to wait for v3 (which some of us might not be able to upgrade to immediately)?

We've just upgraded to 2.9 and then stumbled into this which is a bit of a pain.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 5, 2018

3.0 should be available next week.

@chriskrycho
Copy link

3.0 arriving soon is great, but allow me to second @OliverJAsh's request for a backport to the 2.9 series; I have a library that this broke and would strongly prefer not to have to set a minimum requirement of 3.0 for people consuming its types.

@akosyakov
Copy link

I still see #24599 (comment) for tsx files with next typescript.

resir014 added a commit to kata-ai/aksara-ui that referenced this issue Sep 11, 2018
A downgrade is necessary to prevent this issue from happening:
microsoft/TypeScript#24599
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Declaration Emit The issue relates to the emission of d.ts files Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.