search

LionC / express-basic-auth

Plug & play basic auth middleware for express
325 stars 57 forks source link

@types/express should be moved under devDependencies #4

Closed attila123 closed 6 years ago

attila123 commented 6 years ago

Hi, I think @types/express should be moved under devDependencies in package.json, because it is not necessary to run the module in production, rather it s needed only during development. Due to this issue, some @types/... modules remain installed even after npm prune --production (which is supposed to remove the devDependencies), which is not so nice. What do you think?

LionC commented 6 years ago

Hi attila,

thanks for your input! That sounds like it makes sense - do you want to open a Pull Request or should I just do it?

attila123 commented 6 years ago

Hi LionC, I'd appreciate if you did this, thanks.

toverux commented 6 years ago

Yep, putting it in the deps is not the best idea I've had. I did it because, if the consumer does not install it manually, it will get an error about "express not being a module". But I think now that it is "expected", and can avoid other dependency problems. Moreover, someone who uses express-basic-auth should already have @types/express in its own deps. Also, peerDeps would have been a good choice if it would not force JS users to install it too.

attila123 commented 6 years ago

Hi @toverux , I am not sure I understand correctly what you wrote. Do you mean that if @types/express is not installed in production then it will result in an "express not being a module" error? I though that @types/express is just some kind of type hint for typescript, and therefore it is only needed for developers, and it is not needed in production (for the transpiled code). Or do you mean the use case when typescript files are directly used in production? (I think I read something that it is possible somehow, but our project does the transpiling in build time to ES6, so we don't use .ts files in production.) Unfortunately I am not familiar with peerDeps.

toverux commented 6 years ago

No, I don't refer to production ;) If you don't have @types/express in your own project (in development) and that express-basic-auth removes it, TypeScript won't understand those express references (in express-basic-auth.d.ts). And emit an error like "Could not find a declaration file for module 'express'".

I'll do some tests concerning that asap and create the PR myself (unfortunately, TS' docs do not offer much guidance about declaration files publication outside of DefinitelyTyped).

Thanks for reporting the issue - since anyway, having @types/express in dependencies is definitely not a good approach.

LionC commented 6 years ago

Thanks for taking care of the PR Morgan!

From: notifications=github.com@lionc.de notifications=github.com@lionc.de on behalf of Morgan Touverey-Quilling notifications@github.com Sent: Tuesday, October 3, 2017 6:50:48 PM To: LionC/express-basic-auth Cc: Leon Strauss; Comment Subject: Re: [LionC/express-basic-auth] @types/express should be moved under devDependencies (#4)

No, I don't refer to production ;) If you don't have in your own project (in development) and that express-basic-auth removes it, TypeScript won't understand those express references. And emit an error like "Could not find a declaration file for module 'express'".

I'll do some tests concerning that asap and create the PR myself. Thanks for reporting the issue - since anyway, having in dependencies is definitely not a good approach.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/LionC/express-basic-auth/issues/4#issuecomment-333907896, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ADQvOSWZJRrti7jOVttXCAiUn0__wAvUks5somXogaJpZM4PmCll.

fklingler commented 6 years ago

This issue should be closed I think :) :+1:

attila123 commented 6 years ago

Sorry, somehow did not notice that it got solved. Verified with 1.1.3, got fixed, so closing this issue. Thank you very much for fixing this! :)

vjpr commented 5 years ago

@LionC

This issue is wrong.

express-basic-auth.d.ts imports express, and is present when published. TypeScript will therefore search for express/node_modules/@types/express.d.ts when this package is used by something else.

This is breaking my build.

vjpr commented 5 years ago

https://github.com/LionC/express-basic-auth/pull/5#issuecomment-459910146