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

Do not compile ts code of external module #4667

Closed
vvakame opened this issue Sep 6, 2015 · 10 comments
Closed

Do not compile ts code of external module #4667

vvakame opened this issue Sep 6, 2015 · 10 comments
Labels
Bug A bug in TypeScript Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it

Comments

@vvakame
Copy link
Contributor

vvakame commented Sep 6, 2015

#2338 implements npm module lookup (very cool!)

I have a request.
I want to tsc lookup .d.ts first, not .ts.

in above case.
code dependencies are resolved to node_modules/commandpost/index.d.ts -> node_modules/commandpost/lib/index.ts when my code exec import * as commandpost from "commandpost";
I want to tsc resolve node_modules/commandpost/index.d.ts -> node_modules/commandpost/lib/index.d.ts, not .ts.

$ tree node_modules/commandpost
node_modules/commandpost
├── LICENSE.txt
├── README.md
├── dtsm.json
├── index.d.ts
├── lib
│   ├── argument.d.ts
│   ├── argument.js
│   ├── argument.js.map
│   ├── argument.ts
│   ├── command.d.ts
│   ├── command.js
│   ├── command.js.map
│   ├── command.ts
│   ├── index.d.ts
│   ├── index.js
│   ├── index.js.map
│   ├── index.ts
│   ├── option.d.ts
│   ├── option.js
│   ├── option.js.map
│   ├── option.ts
│   ├── utils.d.ts
│   ├── utils.js
│   ├── utils.js.map
│   └── utils.ts
├── package.json
└── tsconfig.json

external ts code can't compile everywhere.
for example.

$ cat node_modules/commandpost/lib/index.ts
/// <reference path="../node_modules/typescript/lib/lib.es6.d.ts" />

"use strict";

try {
... omitted ...

but node_modules/commandpost/node_modules/typescript/lib/lib.es6.d.ts is not exists (typescript is devDependencies

Does TypeScript team has best practise about project structure for npm module?

commandpost

@vvakame
Copy link
Contributor Author

vvakame commented Sep 6, 2015

other case.

when npm package has ambient external module and proper external module both.
tsc lookup proper external first. I think this is unexpected breaking change.

for example.
actual (tsc 1.6.0-beta

$ find . -type f
./index.ts
./node_modules/foo/index.d.ts
./node_modules/foo/lib/bar.d.ts
./node_modules/foo/lib/buzz.ts

$ cat index.ts
import foo = require("foo");
foo.bar.bar();

$ cat node_modules/foo/index.d.ts
declare module 'foo' {
    export import bar = require("foo/lib/bar");
}

declare module 'foo/lib/bar' {
    function bar(): void;
}

$ cat node_modules/foo/lib/bar.d.ts
import * as buzz from "./buzz";
export function bar(): void;

$ cat node_modules/foo/lib/buzz.ts
export class Sample {
    private foo: NotInTopLevelContextButRequiredInFooPackageContext;
}

$ tsc --listFiles --noImplicitAny --target ES5 --module commonjs index.ts node_modules/foo/index.d.ts
node_modules/foo/lib/buzz.ts(2,15): error TS2304: Cannot find name 'NotInTopLevelContextButRequiredInFooPackageContext'.
/Users/vvakame/work/TypeScript/lib/lib.d.ts
node_modules/foo/lib/buzz.ts
node_modules/foo/lib/bar.d.ts
node_modules/foo/index.d.ts
index.ts

expected

$ tsc --listFiles --noImplicitAny --target ES5 --module commonjs index.ts node_modules/foo/index.d.ts
/Users/vvakame/work/TypeScript/lib/lib.d.ts
node_modules/foo/index.d.ts
index.ts

@vvakame vvakame changed the title Do not compile external module ts code Do not compile ts code of external module Sep 6, 2015
@vvakame
Copy link
Contributor Author

vvakame commented Sep 6, 2015

#4665

@mhegazy mhegazy added the Bug A bug in TypeScript label Sep 6, 2015
@mhegazy mhegazy added this to the TypeScript 1.6.2 milestone Sep 6, 2015
enlight added a commit to debugworkbench/core-components that referenced this issue Sep 13, 2015
dtsGenerator is no longer used to generate the type definitions for this
package, it will be removed from the dependencies list in the near
future if the "proper external module" format doesn't result in any
unforeseen issues.

This change also made it necessary to re-introduce the `src` directory
for `.ts` source files due to an issue with the TypeScript compiler
<microsoft/TypeScript#4667> where TypeScript
would attempt to build the `.ts` files in this package while building
a project that consumes this package (instead of simply referencing the
`.d.ts` files in this package). The workaround for now is to ensure the
`.d.ts` files `.ts` files are in separate directories.
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 15, 2015
@mhegazy mhegazy closed this as completed Sep 15, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Sep 15, 2015

this should be in master and release 1.6, @vvakame can you give it a quick try.

@vvakame
Copy link
Contributor Author

vvakame commented Sep 17, 2015

@mhegazy It is not works fine still...

repro

$ git clone https://github.com/vvakame/review.js.git
$ cd review.js
$ npm install && grunt setup
$ tsc -v
message TS6029: Version 1.6.2
$ tsc -p lib
resources/grammar.d.ts
lib/js/exception.ts
typings/jquery/jquery.d.ts
typings/i18next/i18next.d.ts
lib/parser/walker.ts
lib/parser/parser.ts
lib/parser/analyzer.ts
lib/parser/validator.ts
lib/controller/configRaw.ts
lib/builder/textBuilder.ts
lib/builder/htmlBuilder.ts
node_modules/typescript/lib/lib.es6.d.ts
typings/node/node.d.ts
lib/typings/polyfill.d.ts
lib/typings/custom-colors.d.ts
lib/controller/config.ts
lib/parser/preprocessor.ts
lib/controller/controller.ts
lib/index.ts
lib/utils/utils.ts
lib/i18n/en.ts
lib/i18n/ja.ts
lib/i18n/i18n.ts
lib/model/compilerModel.ts
lib/builder/builder.ts
typings/update-notifier/update-notifier.d.ts
typings/js-yaml/js-yaml.d.ts
node_modules/commandpost/lib/utils.ts      <- not expected, use utils.d.ts instead of utils.ts
node_modules/commandpost/lib/option.ts        <- not expected, use option.d.ts instead of option.ts
node_modules/commandpost/lib/argument.ts   <- not expected, use argument.d.ts instead of argument.ts
node_modules/commandpost/lib/command.ts  <- not expected, use command.d.ts instead of command.ts
node_modules/commandpost/lib/index.ts         <- not expected, use index.d.ts instead of index.ts
node_modules/commandpost/index.d.ts     <- works fine!
lib/cli.ts
lib/utils/polyfill.ts
typings/assert/assert.d.ts
typings/mocha/mocha.d.ts
typings/bundle.d.ts

@mhegazy mhegazy reopened this Sep 18, 2015
@mhegazy mhegazy removed the Fixed A PR has been merged for this issue label Sep 18, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Sep 18, 2015

@vladima can you take a look.

@vladima
Copy link
Contributor

vladima commented Sep 18, 2015

@mhegazy this is a known issue with transitive imports from the 'external typings only' contexts that was not yet fixed

vvakame added a commit to vvakame/commandpost that referenced this issue Sep 21, 2015
vvakame added a commit to vvakame/review.js that referenced this issue Sep 22, 2015
@vvakame
Copy link
Contributor Author

vvakame commented Sep 22, 2015

vvakame/commandpost@d637859
this is my workaround. it works fine 👍
I love tsc 1.6 😸

@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

After going on this back and forth a few times, the current behavior seems to be the best. Users working on multiple modules at design time would like to get this behavior.
@vladima has a tool to verify a package with "recommended" setup, that interested package authors can run, instead of the compiler enforcing it.

@mhegazy mhegazy closed this as completed Jan 7, 2016
@mhegazy mhegazy added the Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it label Jan 7, 2016
@adidahiya
Copy link
Contributor

@mhegazy cool, is that tool available somewhere?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 7, 2016

@vladima can you share your tool?

kenju added a commit to bitjourney/react-simple-image that referenced this issue Sep 11, 2017
because tsc can look up the *.tsx? files mistakenly

see microsoft/TypeScript#4667
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it
Projects
None yet
Development

No branches or pull requests

4 participants