Skip to content
This repository has been archived by the owner on Jun 29, 2021. It is now read-only.

Unnecessary package.json dependencies #47

Closed
MichalLytek opened this issue Oct 8, 2017 · 7 comments
Closed

Unnecessary package.json dependencies #47

MichalLytek opened this issue Oct 8, 2017 · 7 comments

Comments

@MichalLytek
Copy link
Contributor

MichalLytek commented Oct 8, 2017

Why mongoose is a real dependency of typegoose? It should be a peerDependency and users should install it by themselves, as they still need to explicitly use it by themselves:

import * as mongoose from 'mongoose';
mongoose.connect('mongodb://localhost:27017/test');

So when they use different version than the typegoose one, there will be problems with no errors marked by npm - two copies of mongoose in one project.

Also, you distribute on npm the compiled version and "main" points to "lib/typegoose.js" which is correct. So why you have typescript listed in dependecies? Lib consumers doesn't need this, they can use the lib even with flow+babel, as long as they produce api-compatible js transpiled code.

import 'reflect-metadata'; is also not needed in typegoose source - it's only a runtime shim for Reflect.metadata to make TS decorator class type reflection works. Again, users should import it in their entry file as some of them might want to use core-js polyfill instead.

@szokodiakos
Copy link
Owner

Hello. Sounds good, less dependencies mean less problems. If you have the time a PR of this would be greatly appreciated. If tests pass and the package remains completely usable then it is fine with me.

@AmazingTurtle
Copy link

The reflect-metadata one is a bug.
Typegoose imports reflect-metadata. InversifyJS also requires it. So I imported it aswell.
I did not expect typegoose to import it internally aswell!

This caused me cancer and brainfuck.
If you use both InversifyJS and Typegoose, one must remove the own import of reflect-metadata or else it'll lead to weired errors.
Please leave the import of reflect-metadata to the users. Please.

@Ben305
Copy link
Collaborator

Ben305 commented Mar 27, 2018

We cannot remove the import because of https://github.com/szokodiakos/typegoose/blob/master/src/prop.ts#L176

@AmazingTurtle
Copy link

@Ben305 I see you're casting to any here. No strict typing. Therefore reflect-metadata is not required for passing a build.
I say: remove the import and leave it to the user, because other packages are importing reflect-metadata aswell - thus causing an error that's not so nice to resolve.

@Ben305
Copy link
Collaborator

Ben305 commented Mar 27, 2018

We cannot remove it, because:

TypeError: Reflect.getMetadata is not a function
    at /Users/Ben305/Projects/typegoose/lib/prop.js:9:7841

Without casting to any, the typescript compiler throws an error.

I'd like to see a PR if you get it working.

@MichalLytek
Copy link
Contributor Author

MichalLytek commented Mar 27, 2018

@Ben305
reflect-metadata is a polyfill, typegoose shouldn't import it by itself

@MichalLytek
Copy link
Contributor Author

Closing this as it's out of date.

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

No branches or pull requests

4 participants