Skip to content

Moved @types to devDependencies. #384

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
wants to merge 1 commit into from
Closed

Moved @types to devDependencies. #384

wants to merge 1 commit into from

Conversation

illuxio
Copy link

@illuxio illuxio commented Jan 15, 2017

@types are not necessary at runtime. Therefore, @types should be a devDependency.

@blakeembrey
Copy link
Member

Not true. Please read the existing discussions.

@aciccarello
Copy link
Collaborator

@illuxio See #335, #350, and #310 (review) for prior discussions.

@illuxio
Copy link
Author

illuxio commented Jan 21, 2017

@blakeembrey After reading the prior discussions I'm speechless. Enforcing build dependencies that a user may need, would be for me like forcing the user to install another OS because he/she may need it. Other project for example based on compiling c have a readme that states which additional resources you may need and yes you have to take care of.

Edit: By the way. With this you force users also to use maybe old typing that may not fit after a package update 👎

@blakeembrey
Copy link
Member

blakeembrey commented Jan 21, 2017

@illuxio That is a pretty bad argument and has zero relation to an OS. They're regular dependencies. I can't help it if you don't happen to use that code path. How would you like it if every NPM dependency you installed you have to read through their package.json and install the dependencies yourself?

@blakeembrey
Copy link
Member

blakeembrey commented Jan 21, 2017

@illuxio What exactly is your problem? You've put absolutely zero effort into explaining why this PR is remotely valid and really have just wasted my time by insulting instead of being constructive. If you can't understand something, you should learn about it yourself or try it out before assuming you're correct. If you think it's best that projects ship with zero dependencies and you should have to re-install everything every time, each to their own, but that's not how the node ecosystem or NPM works.

@illuxio
Copy link
Author

illuxio commented Jan 21, 2017

Actually I would like it, because when I use a package as library I have to read a documentation anyway!

But in order to find a solution. How about putting @types in the optional dependency section?

@blakeembrey
Copy link
Member

@illuxio No, they are a dependency. Please explain what problem you're having so I can help you find an appropriate solution first, that'll be more productive here.

With this you force users also to use maybe old typing that may not fit after a package update

That shouldn't matter though, since you aren't forced to use them - they are only for typedoc.

@blakeembrey
Copy link
Member

It seems to me that, as I noted in previous discussions, your issue would be with DefinitelyTyped being incorrect. For example, I found that https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d568404f00bf57c1f00c9cdb71545d77d1480471/handlebars/index.d.ts#L293 is still written in global format. To be correct, this should really be written in external module format. If that were done, none of our dependencies would be incorrectly global. The fact that the default behaviour of TypeScript is to pick up these globals is an unfortunate behaviour that I dislike and have comment to TypeScript about. But these issues are related to TypeScript's implementation of modules, and forcing every module author to re-install every sub-dependency would put us back in the days of TSD which is not productive for anyone.

@illuxio
Copy link
Author

illuxio commented Jan 21, 2017

One of my main problems is the "Duplicate identifier" error given by typescript, because two packages include different version of their @types as dependencies. For example @types/[email protected] and @types/[email protected]. However, I am only using [email protected] and know that the @types/[email protected] is irrelevant to me and makes only trouble. How would you solve this issue?

I really would love a good solution that does not involve "hacky" tsconfig or post deletion of packages from the node_module folder.

@blakeembrey
Copy link
Member

@illuxio Thanks, that makes sense now. So, all @types dependencies should be written as external modules (if they're external modules) and that's the issue here - some legacy DefinitelyTyped stuff. It's really unfortunate that TypeScript decided to use @types for everything instead of separate namespaces like I did with Typings, but we just have to live with it now - the unfortunate side-effect is published @types might be global or external. External ones are what we really want to be using, because they would not cause any conflicts for you.

The latest lodash appears that it's written correctly - see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/lodash/index.d.ts. Do you know where [email protected] comes from?

@blakeembrey
Copy link
Member

From my manual exploration, it seems from our dependencies highlight.js, marked and handlebars are still written incorrectly and would need to be updated in DefinitelyTyped. I'm not sure why Lodash would be causing an issue at this time for you, unless it's coming from a different package?

@illuxio
Copy link
Author

illuxio commented Jan 21, 2017

As I mentioned it was an example. At the current state lodash is no issue and back then when I had this issue I removed the causing package from our package.json because we weren't using it anymore. But this was still in my mind.

But thanks for your time. I will think about what you said.

@blakeembrey
Copy link
Member

blakeembrey commented Jan 21, 2017

I understand the concern, believe me, but it's detrimental to enforce on module authors as it makes the development experience so much worse. The real issue, to me, is the fact that these conflicts can simply occur at any time thanks to the TypeScript compiler and dependency set up. If there were two definition namespaces (global and modules), then this entire discussion could have been averted. Don't worry about it too much though, I've spent countless hours describing the issues that exist to others too, I just strongly disagree the solution would be to make development suck any more than it needs to.

@hdeshev
Copy link

hdeshev commented Feb 25, 2017

Another guy that got bitten by the "@types/* as dependencies" issue here.

I realize you want to make people's life easier when they want to use typedoc as a library, but it seems that is hurting the majority of users that just call the binary to get their API reference right. Would it be better if there was a documentation section that tells "library" users which @types dependencies they need to reference?

Would you consider accepting a PR that adds a script installing the extra dependencies, so "library" users could run ./node_modules/.bin/typedoc-dev-dependencies and have all necessary dev dependencies set up for them automatically?

@aciccarello
Copy link
Collaborator

@hdeshev thanks for offering another solution but I don't think this is TypeDoc's problem. Isn't the real solution is fixing the @types definitions for everyone?

@hdeshev
Copy link

hdeshev commented Feb 25, 2017

@aciccarello, I think we both agree that:

  1. This is a problem, and it affects TypeDoc users.
  2. This isn't TypeDoc's problem.

That doesn't help people like me very much though. I still had to fork TypeDoc in order to fix NativeScript's documentation build. That is the only thing I could do, since, even if I'd forked the broken lodash typings, I would still have had to fork TypeDoc in order to make it pull in my fixed version of those typings.

I think that there is value in solving the problem in a way that will:

  1. Reduce the amount of packages most people download.
  2. Eliminate the chance of future type clashes and other problems inflicted by bad typings for most end users like me.

All that at the tradeoff of having to document the typing dependencies needed by people who use TypeDoc as a library (TypeDoc development shouldn't be affected, as those dev dependencies will make sure the typings are available.). So, to rephrase my question from the previous comment: is this an acceptable tradeoff?

@aciccarello
Copy link
Collaborator

@hdeshev Right, so the main problems that we are trying to solve are the "type clashes and other problems inflicted by bad typings for most end users". My gut reaction is that users should submit PRs to DefinitelyTyped to fix the broken typings as that is the source of the issue. Generally, this isn't too hard and helps the larger community.

Manually installing the @types dependencies of TypeDoc may seem trivial but I agree that fundamentally you shouldn't have to do that to use it as a library. It gets more and more complicated with multiple levels of dependencies and different versions of libraries being used.

But still that does leave a period of time while type definitions are being fixed when TypeDoc is unusable therefore I think we should have a workaround.

Ideally I would hope that we could make the workaround opt-in. So if you are having trouble with TypeDoc while a type definition is being fixed you could change some configuration which prevents the conflict issues. This would temporarily prevent the error but indicates that there needs to be a more complete solution. That's what I'm thinking at the moment, but I'd be open to discussing it further.

@illuxio
Copy link
Author

illuxio commented Mar 23, 2017

@blakeembrey: Hi it's me again,

today I had to face the following issue:

node_modules/@types/lodash/index.d.ts(12762,29): error TS2304: Cannot find name 'object'.
node_modules/@types/lodash/index.d.ts(19507,15): error TS2428: All declarations of 'WeakMap' must have identical type parameters.
node_modules/@types/lodash/index.d.ts(19507,33): error TS2304: Cannot find name 'object'.

Currently we have to using typescript 2.1 and therefore use a proper typedoc version (currently 0.5.5). However, due to the recent update of @types/lodash (which require ts 2.2) our build is failing. But the typing is just a dependency of typedoc and we do not need it. :(

Any suggestions?

@blakeembrey
Copy link
Member

@illuxio I'm really sorry, I hate this auto-updating and breaking users approach to @types. Unfortunately it's just what TypeScript has given us. Can you open an issue on TypeScript or DefinitelyTyped about this, they need to see that what they're doing - with the lack of real semver - is breaking users. On the TypeDoc side, would it help if we pin the versions of @types we're using?

@illuxio
Copy link
Author

illuxio commented Mar 24, 2017

@blakeembrey: There are some issues occurring that states my problem e.g. DefinitelyTyped/DefinitelyTyped#14384 . However the solution is sub-optimal due to the lost of version control with the tags.

Form typedoc side, pinning sound reasonable due to the relation between typedoc and typescript. I would like this solution.

@aciccarello
Copy link
Collaborator

If we pin the @types versions, is there any way for us to test that we won't cause errors for users when we do update?

@dehru
Copy link

dehru commented Mar 24, 2017

Hello, I'm having this same error with the WeakMap as mentioned above.

TypeDoc 0.5.9 
"All declarations of 'WeakMap' must have identical type parameters."

@dehru
Copy link

dehru commented Mar 24, 2017

Would it help for me to open a new issue instead of us all commenting on this issue which is unrelated?

@blakeembrey
Copy link
Member

Anyone can submit a PR to pin the versions for now. It'd be useful if someone, other than me, wants to pester TypeScript about this issue 😄 I think I've complained enough to them about the lack of real semver in the @types implementation.

@dehru
Copy link

dehru commented Mar 24, 2017

I'm a typescript newbie. Would I pin down the version of @types/lodash or typescript?

@aciccarello
Copy link
Collaborator

It would be the @types you'd pin down

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

Successfully merging this pull request may close these issues.

5 participants