@Types are stored as dependencies in package.json causing conflicts

[richardspence]

@types are stored as dependencies in package.json causing conflicts.

These conflicts are due to @types/node being added which may conflict with the locally defined global variables.

They should be moved to the dev dependencies.

[Gerrit0]

We should definitely fix this before the next release. Should be a really simple fix.

[aciccarello]

I believe these are actually needed for TypeDoc to be fully typed as a dependency. Blake Embrey, who has done a lot of work around types has been pretty adamant about that. It'd be great if this weren't true because of the conflicts with local types but my understanding is that they need to stay as direct dependencies. Please let me know if there is a better solution.

Duplicate of #408 #335 #350 #384 #418, #434, and #600

Please see those issues for discussion.

[Gerrit0]

The more you know...

Again, this is not about compiling the typedoc package. This is for other people using it programmatically with TypeScript. - Blake

I'd like to take a closer look at this. I don't believe anything from @types/shelljs is actually exported by TypeDoc, so it shouldn't be required. Unless we export types that are imported from one of these modules, they shouldn't be included. We use fs-extra internally, but I don't think it is exported so it shouldn't be included.

[richardspence]

Another option is to split the package.

Have typedoc have the optional dependency of @types/typedoc so consumers can install the later if they require strongly typed programatic access.

Having typedoc install typings is overly opinionated and have burdensome repercussions for consumers esp when CLI use is desired.

[aciccarello]

Those are good points. I agree that it is burdensome and would like to fix it. I'd need to dig into the use cases more but I like the idea of trying to move the non-essential @types dependencies.

[richardspence]

I also haven't checked but @types uses package refernces via an xml comment tag /// <references path="foo" /> . It might work in-project-typings.

[blakeembrey]

Just to clarify, I think global typings such as node should be dev dependencies. Those assertions are only true for actual dependencies you may expose transitively and wouldn’t otherwise conflict.

Edit: It sounds like a transitive dependency on node here? That’s really unfortunate. I’d try to convince Microsoft/TypeScript that global dependencies shouldn’t be dependencies since that means not even this package can easily be developed against if they conflicted.

[aciccarello]

Yeah, we don't have a direct dependency on @types/node but the types are added.

[email protected] ./typedoc
├─┬ @types/[email protected]
│ └── @types/[email protected] 
└─┬ @types/[email protected]
  ├─┬ @types/[email protected]
  │ └── @types/[email protected]  deduped
  └── @types/[email protected]  deduped

So it seems like we need to review what types are actually exposed. I haven't looked thoroughly but just looking at the MarkedPlugin class, the four typed packages that are imported don't appear in the final .d.ts

// MarkedPlugin.ts
import * as FS from 'fs-extra';
import * as Path from 'path';
import * as Marked from 'marked';
import * as HighlightJS from 'highlight.js';
import * as Handlebars from 'handlebars';

// MarkedPlugin.d.ts
import { ContextAwareRendererComponent } from '../components';
import { RendererEvent, MarkdownEvent } from '../events';
export declare class MarkedPlugin extends ContextAwareRendererComponent {
    includeSource: string;
    mediaSource: string;
    private includes?;
    private mediaDirectory?;
    private includePattern;
    private mediaPattern;
    initialize(): void;
    getHighlighted(text: string, lang?: string): string;
    parseMarkdown(text: string, context: any): string;
    protected onBeginRenderer(event: RendererEvent): void;
    onParseMarkdown(event: MarkdownEvent): void;
}

The unfortunate thing is that this would need to be a manual check to find what is actually exposed unless we can come up with a simple test repo.

[richardspence]

RE: Manual testing.

Here's a test repo that should work (completely untested):
If it compiles, it passes. This could be added to your test suites for automatic testing

CMD

tsc --project ./tsconfig.globalvars.json

tsconfig.globalvars.json

{
"extends": "./tsconfig",
"include": ["**/*.ts", "./tests/globalVarTest.test"]
}

./tests/globalVarTest.test

define global{
// use require since it's a common global variable that can have conflicts.  Other candidates can include console, jquery, underscore.
var require : string;
}

[aciccarello]

Thanks @richardspence. That'd be a good check to have specifically for node. I'm also looking into checking the generated .d.ts files from typedoc to see if they contain any external imports so we know what (if any) types are exported and need to be included as regular dependencies.

From what I've seen, TypeScript just uses any on any imports that don't have types so it's not easy to figure out what might be missing. 🤔

[demurgos]

Hi,
I just wanted to add that I definitely agree with @blakeembrey : @types/node should be in devDependencies. Since it defines the runtime environment it should be explicitly provided by the top level consumer (this is similar to lib in tsconfig.json). I posted a comment with more details here but did not receive any reply from TS maintainers yet.

[richardspence]

@aciccarello RE: any on any imports that don't have types... Turning on noImplicitAny in the compiler options would block that at compile time - but it could also block a lot of other instances in code and require cleanup before use.

[richardspence]

@aciccarello Thanks for closing this. Any ETA on the next release? Can we publish this as an alpha version earlier?

[aciccarello]

We are working on fixing a bug on master. Then we'll get an alpha release out. Thanks for your patience!