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

Need warn about ts files (not dts) in compilation from node_modules #53

Closed
rhuitl opened this issue Oct 26, 2018 · 17 comments
Closed

Need warn about ts files (not dts) in compilation from node_modules #53

rhuitl opened this issue Oct 26, 2018 · 17 comments
Assignees
Milestone

Comments

@rhuitl
Copy link

rhuitl commented Oct 26, 2018

I would like to include the typings for https://github.com/Lusito/typed-signals in my DTS bundle, however the extraction fails with "Error: Cannot find symbol for node: emitCollecting".

app.ts:

import {Signal} from "typed-signals";

export const signal: Signal<(argument) => void> = undefined;

package.json:

{
  "name": "dts-bundle-generator-bugreport",
  "version": "1.0.0",
  "private": true,
  "scripts": {
    "build": "npx tsc",
    "typings": "npx dts-bundle-generator -o app.d.ts app.ts"
  },
  "dependencies": {
    "typed-signals": "^1.0.2"
  },
  "devDependencies": {
    "dts-bundle-generator": "^1.6.1",
    "typescript": "^3.1.3"
  }
}

tsconfig.json:

{
  "compilerOptions": {
    "target": "es5",
    "declaration": true,
    "lib": ["es2015", "dom"]
  }
}

Run:

npm install
npm run typings

Output:

Error: Cannot find symbol for node: emitCollecting

The method looks like this (see https://github.com/Lusito/typed-signals/blob/master/src/Signal.ts):

export class Signal<CB extends Function> {
  // ...
  protected emitCollecting<RT>(collector: Collector<CB, RT>, args: any) {
    // ...
  }
}
@timocov
Copy link
Owner

timocov commented Oct 27, 2018

Hi,

the error happens for (signal as any).emitCollecting expression.

But anyway it is bug and I'll fix it.

@timocov timocov added the Bug label Oct 27, 2018
@timocov timocov self-assigned this Oct 27, 2018
@timocov
Copy link
Owner

timocov commented Oct 27, 2018

@rhuitl There is some other thoughts about it: the library typed-signals in npm is as TypeScript code, not d.ts files + js files.

To remove js-output-specific stuff (like function's body, full class' implementation and so on) dts-bundle-generator build your input in d.ts (it did it in-memory) first, and then compile these d.ts to operate with types/declarations only. It didn't expect that in declarations we'll have implementation of something, and the TypeScript does not compile code from node_modules so we cannot get types/declarations for that modules.

As workaround (and, actually, right solution) I believe that typed-signals shouln't provide their TypeScript source code in npm, instead it should provide both d.ts declarations for type checking and built js files.

@timocov
Copy link
Owner

timocov commented Nov 1, 2018

So, according my previous comment I don't think that it is actually bug. We can try to warn about such situations, but I'm not sure whether we should fix it in this tool.

@timocov timocov added Enhancement and removed Bug labels Nov 1, 2018
@timocov timocov changed the title Error: Cannot find symbol for node: emitCollecting Need warn about ts files (not dts) in compilation from node_modules Nov 1, 2018
@rhuitl
Copy link
Author

rhuitl commented Nov 1, 2018

Yes you are right, with the correct file in the typings field, dts-bundle-generator works just fine. We could as well close this ticket now :) Thanks for your support.

@aleclarson
Copy link

The tsc executable has support for non-transpiled dependencies, so why shouldn't dts-bundle-generator? For example, if I import types from a non-transpiled dependency, tsc still generates the .d.ts modules without complaint.

@timocov
Copy link
Owner

timocov commented Jun 5, 2019

if I import types from a non-transpiled dependency, tsc still generates the .d.ts modules without complaint.

It does? Wow, I didn't know that! If it's really does it, I believe this bug can be fixed.

But just for an information - what's a case you publish non-transpiled sources? Who'll execute/compile it when somebody install it? I'm not sure if it's correct to do this.

@aleclarson
Copy link

@timocov
Copy link
Owner

timocov commented Jun 5, 2019

Oh, I see. Just add --disable-symlinks-following CLI option and it looks like this fixes the problem.

@timocov
Copy link
Owner

timocov commented Jun 5, 2019

Why do you decide that TypeScript compiler compiles node_modules folder? I see that after building foo package bar doesn't have index.d.ts anywhere.

@aleclarson
Copy link

Oh, I see. Just add --disable-symlinks-following CLI option and it looks like this fixes the problem.

If you ask me, inlining should be opt-in. But yes, that is a good workaround. 👍

@timocov
Copy link
Owner

timocov commented Jun 5, 2019

If you ask me, inlining should be opt-in.

What do you mean?

@aleclarson
Copy link

What do you mean?

By default, when dts-bundle-generator finds a symlink, it follows the symlink and includes its type definitions in the .d.ts bundle. But that should only happen for symlinks that point to internal modules. Symlinks to external modules should be treated as if --disable-symlink-following was given.

Why do you decide that TypeScript compiler compiles node_modules folder? I see that after building foo package bar doesn't have index.d.ts anywhere.

Yeah, I didn't mean to imply that. Notice how tsc doesn't throw an error when a dependency's main module isn't compiled. I think dts-bundle-generator should work the same way. No warnings, no errors.

@timocov
Copy link
Owner

timocov commented Jun 5, 2019

Notice how tsc doesn't throw an error when a dependency's main module isn't compiled

I can't imagine a case where it's ok, even for monorepos, because it would be better to have compiled .d.ts files instead of .ts ones to avoid unnecessary compilation of source files which wouldn't generate an output.

Also, I believe that in case of monorepo you need to compile dependencies first and generate .d.ts files for them (it'll reduce a building time as well, because parsing .d.ts is easier than parsing and compiling .ts file).

Also that warning is about .ts files in node_modules folder, which (I believe) shouldn't be even exist in common case of using npm/yarn (because a packages should provide declaration files, not source files as declaration ones).

By default, when dts-bundle-generator finds a symlink, it follows the symlink and includes its type definitions in the .d.ts bundle.

Not actually. Resolving symlinks are for the TypeScript compiler. After the compiler creates the program, dts-bundle-generator uses files it provides to work with them, and there is real paths, not import/require's ones. For now dts-bundle-generator treats file as external by its path (so, if tsc resolves an import to original file located in the same folder - dts-bundle-generator will treat it as local one, not external; but if no symlinks resolving happened, then a file's path will contain node_modules path and the tool will treat it as external).

Symlinks to external modules should be treated as if --disable-symlink-following was given.

How to detect that some module is external one? It looks like we have a path of the file only for that.

@aleclarson
Copy link

aleclarson commented Jun 5, 2019

Also, I believe that in case of monorepo you need to compile dependencies first and generate .d.ts files for them (it'll reduce a building time as well, because parsing .d.ts is easier than parsing and compiling .ts file).

In my scenario, each package contains both package.json and dist/package.json modules. When linked together, the packages use package.json instead of dist/package.json, because I build them all in parallel (which speeds up builds a ton).

If they instead used each other's dist/package.json module, I wouldn't be able to build in parallel. Otherwise, a package that depends on another package in the monorepo might try to install it before its dist directory exists.

Also that warning is about .ts files in node_modules folder, which (I believe) shouldn't be even exist in common case of using npm/yarn (because a packages should provide declaration files, not source files as declaration ones).

In my scenario, the uncompiled .ts modules are only used in local development. Before publishing, they are replaced with the compiled dependencies.

How to detect that some module is external one? I looks like we have a path of the file only for that.

Hmm, you could probably gather the import statements from the AST of each module. Anyway, it's probably not worth the effort for now.

@timocov
Copy link
Owner

timocov commented Jul 14, 2019

I'm back after looong time :)

because I build them all in parallel (which speeds up builds a ton).

Hm, it's a little bit unexpected for me. I believe it speeds up builds, but it confuses.

Hmm, you could probably gather the import statements from the AST of each module

Yeah, I need. There is a lot of issues caused by that - https://github.com/timocov/dts-bundle-generator/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+label%3Adetect-import-path-problem

I wouldn't be able to build in parallel.

I believe in this case you need to have resolver/scheduler which will make a build plan and detect which packages can be built in parallel.

I just dig into

Notice how tsc doesn't throw an error when a dependency's main module isn't compiled. I think dts-bundle-generator should work the same way

and it seems that there is no way to get dts file for module from node_modules directory. The compiler doesn't provide a way to generate declaration file for any source file (like transpile or transpileModule to get JS output).

If you ignore that warning, does it work correct for you or you get some errors?

/cc @aleclarson

@aleclarson
Copy link

Hey @timocov, shortly after trying your tool, I decided to start contributing to rollup-plugin-dts and that's serving my needs well. I even figured out how to get the TypeScript compiler to generate .d.ts modules from .ts modules in-memory, which you might be interested in (see here).

Since I don't need your library now, I won't be able to justify helping you out. Sorry!

@timocov
Copy link
Owner

timocov commented Jul 14, 2019

which you might be interested in (see here).

dts-bundle-generator does it already from the first version I believe 🙂 But thanks for pointing this!

Since I don't need your library now, I won't be able to justify helping you out. Sorry!

Ok, no problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants