Skip to content

Discovering .ts files under node_modules resulting in them being compiled (2) #22228

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
evil-shrike opened this issue Feb 28, 2018 · 12 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@evil-shrike
Copy link

There's the issue #6964. It's closed and "fixed".
But I still have this problem in TS 2.7.1

There's an app with the following tsconfig.json which has a dependency to a lib. The lib is installed via npm into node_modules. When the app is being compiled then lib is compiled as well.
It's undesirable.

{
    "extends": "./node_modules/@croc/webclient/tsconfig",
    "include": [
        "src/**/*.ts",
        "src/**/.*.ts"
    ]
}

As you can see the tsconfig includes all app files but no lib's files. So I don't want the lib in node_modules was compiled.

Expected behavior:
No compilation happen for lib files inside node_modules.

Actual behavior:
Compilation does happen.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 1, 2018

add "exclude" with ["node_modules"]?

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Mar 1, 2018
@evil-shrike
Copy link
Author

@mhegazy it seems not to help in this case -
app's tsconfig:

{
    "extends": "./node_modules/@croc/webclient/tsconfig",
    "include": [
        "src/**/*.ts",
        "src/**/.*.ts"
    ],
    "exclude": [ "node_modules" ]
}

I still getting js/map files generated in installation folder inside node_modules
and run tsc --listEmittedFiles prints paths from node_modules

@evil-shrike
Copy link
Author

Do you need a simple demo for reproducing?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2018

do you have an import to a module in the package that is being built? if so then that is expected.. see https://github.com/Microsoft/TypeScript/wiki/FAQ#why-is-a-file-in-the-exclude-list-still-picked-up-by-the-compiler for more details.

it seems that this is a bug in the package that you are importing, a .ts file should not be exported from a package.

to work around this add a "paths" entry for the module in question to redirect it to some local .d.ts, but ultimately the package author should fix it.

@evil-shrike
Copy link
Author

evil-shrike commented Mar 3, 2018

do you have an import to a module in the package that is being built?

yes, the app imports modules from the lib. And only that imported modules are being compiled.

it seems that this is a bug in the package that you are importing, a .ts file should not be exported from a package.

Not sure I understand. Why is it a bug? The app is written in TS and the lib is written in TS, of cause the app imports TS modules from the lib.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 3, 2018

the lib has either "types": "index.ts" or a .ts file at its root. which is wrong. a library should not expose its sources, only its declarations.

First, as a library author you do not want your users to recompile your sources every time they compiler their code. this is a lot of wasted compile time. second, the library author may not be using the same setting as their user do, e.g. --noImplicitAny.

The compiler thinks of any .ts file as a source that needs to be processed.

@evil-shrike
Copy link
Author

First, as a library author you do not want your users to recompile your sources every time they compiler their code

yes, that's what the issue about
at the same time I want to give users an ability to recompile ts files with their own settings.
for example tsconfig in the lib targets es5 but an app can decide to use es6 and recompile the lib with its target. to do this the app author would include all lib sources in app's tsconfig.

and another argument - the ability to debug ts code - with d.ts users will have to debug js in lib.

The compiler thinks of any .ts file as a source that needs to be processed.

ok, this is the reason.
the compile can't see the border between the app and the lib. but it exists. if we install a package in node_modules it's unlikely that someone expects it to be compiled.
so I'd suggest to stop compilation when the compiler following imports goes inside node_modules.
or at least introduce an option for such a behavior

@mhegazy
Copy link
Contributor

mhegazy commented Mar 3, 2018

at the same time I want to give users an ability to recompile ts files with their own settings.
for example tsconfig in the lib targets es5 but an app can decide to use es6 and recompile the lib with its target.

I do not think this is a good idea.. now if they want to use a new compiler version that causes a new error in your code, what do they do? they can not fix these issues anyways.. and why wast compiler cycles compiling the same code over and over.. that is exactly what .d.ts files are for..

and another argument - the ability to debug ts code - with d.ts users will have to debug js in lib.

that is not true. just include your sources, and soruce maps. just do not make it the resolution location for your package.. e.g. your package can have lib\index.js and lib\index.d.ts and a src\index.ts. in your package.json add "types": "lib\index.d.ts".

the compile can't see the border between the app and the lib. but it exists. if we install a package in node_modules it's unlikely that someone expects it to be compiled.

that is not always the case. in mono-repo projects, ppl have links between their node_modules, and they expect them to all compile.. we have users who depend on that behavior.

@evil-shrike
Copy link
Author

if *.ts always come with d.ts then a question arises - why do you need *d.ts? if only for avoiding compilation then it could be achieved by other ways - as I said by telling the compiler where to stop or ignore node_modules (ok, with a flag)

honestly it's a big surprise for me that libs can't export *.ts and should be recompiled to *.d.ts.
I run compilation of my lib with -d and got tons of errors which didn't happen during normal compilation, so it's another headache for free.

I do not think this is a good idea.. now if they want to use a new compiler version that causes a new error in your code, what do they do?

that would be a problem but it also can happen with *d.ts as well.
it's unfortunate case, they will have to stick to a supported ts version for a while.
but there're other useful cases - I don't have to compile lib for all targets - users can decide which one to use. the same is about using of tslib.

your package can have lib\index.js and lib\index.d.ts and a src\index.ts. in your package.json add "types": "lib\index.d.ts".

why this "types": "lib\index.d.ts" is needed? i have no index.ts I have more then 500 files with complex folder structure.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 3, 2018

if *.ts always come with d.ts then a question arises - why do you need *d.ts? if only for avoiding compilation then it could be achieved by other ways - as I said by telling the compiler where to stop or ignore node_modules (ok, with a flag)

A .d.ts does not have expressions.. it represents the shape of the API. it is significantly smaller version of the .ts file. it takes less time to compile, and more importantly it is the API shape, no errors to be reported here..

A .d.ts describes the part of your package your users care about, i.e. the public API you expose; how that is implemented is not so much of interest here.

I think we are now debating some basic concept of should you export your code to your users or expose a declaration file. Our position on this is you should expose a declaration file. implementation details that do not impact the API contract, should not be exposed to API authors.

@mhegazy mhegazy added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Needs More Info The issue still hasn't been fully clarified labels Mar 3, 2018
@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@ryanki1
Copy link

ryanki1 commented Jul 19, 2018

With this config object from aviabird angular 5 seed project I did not get any compilation errors from "node_modules":

{
-- "compileOnSave": false,
-- "compilerOptions": {
---- "outDir": "./dist/out-tsc",
---- "sourceMap": true,
---- "declaration": false,
---- "moduleResolution": "node",
---- "emitDecoratorMetadata": true,
---- "experimentalDecorators": true,
---- "target": "es5",
---- "typeRoots": [
---- "node_modules/@types"
---- ],
---- "lib": [
---- "es2017",
---- "dom"
---- ]
-- }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants