search

angelnikolov / ts-cacheable

Observable/Promise Cache Decorator
https://npmjs.com/package/ts-cacheable
ISC License
340 stars 42 forks source link

rxjs must be a peer dependency #3

Closed ebrehault closed 6 years ago

ebrehault commented 6 years ago

Hi,

I think it would be better if @types/jasmine and jasmine were in devDependencies and rxjs were in peerDependencies (so it does not install those packages into ./node_modules/ngx-cacheable/node_modules if they are missing).

ebrehault commented 6 years ago

Ok, I should have checked first, this PR is a duplicate of https://github.com/angelnikolov/ngx-cacheable/pull/2

To fix the Travis build, we need to install RxJS manually in .travis.yml (as peerDependencies are not installed when running npm install).

@angelnikolov regarding the comment you made to @dgaletto:

however using any version before 6.0.0 will not be possible, unless you use rxjs-compat

That's exactly why we should use peerDependencies here. Because by using dependencies, if the current project uses RxJS 5, npm will install RxJS 6 into ./node_modules/ngx-cacheable/node_modules and everything will look like it works, but it won't, and our IDE will seem crazy (like complaining about Observable<any> id not Observable<any>) because we will have 2 different version of RxJS in the path.

Using peerDependencies, we will get meaningful errors about the RxJS import.

angelnikolov commented 6 years ago

Thanks for the contribution! Will publish later today