Skip to content

Conversation

@ronjouch
Copy link
Contributor

@ronjouch ronjouch commented Jan 22, 2020

Not including it in devDeps means that strict: true TSC projects
using mongoms currently fail to compile with:

node_modules/mongodb-memory-server-core/lib/MongoMemoryServer.d.ts:3:22 - error TS7016:
  Could not find a declaration file for module 'tmp'.
 'node_modules/mongodb-memory-server-core/node_modules/tmp/lib/tmp.js' implicitly has an 'any' type.
  Try `npm install @types/tmp` if it exists or add a new declaration (.d.ts) file containing `declare module 'tmp';`

3 import * as tmp from 'tmp';
                       ~~~~~

Not including it in devDeps means that `strict: true` TSC projects
using mongoms currently fail to compile with:

```
node_modules/mongodb-memory-server-core/lib/MongoMemoryServer.d.ts:3:22 - error TS7016:
  Could not find a declaration file for module 'tmp'.
 'node_modules/mongodb-memory-server-core/node_modules/tmp/lib/tmp.js' implicitly has an 'any' type.
  Try `npm install @types/tmp` if it exists or add a new declaration (.d.ts) file containing `declare module 'tmp';`

3 import * as tmp from 'tmp';
                       ~~~~~
```
@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

Merging #266 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #266   +/-   ##
=======================================
  Coverage   80.75%   80.75%           
=======================================
  Files           9        9           
  Lines         764      764           
  Branches      144      144           
=======================================
  Hits          617      617           
  Misses        146      146           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac06af3...a17c9f0. Read the comment docs.

@BigForNothing
Copy link
Contributor

@ronjouch see #265

It should be in dependencies btw, not devDependencies as I discussed in that PR.

@hasezoey hasezoey added the discussion This Feature/enhancement still needs to be decided if it is wanted label Jan 22, 2020
@ronjouch
Copy link
Contributor Author

see #265. It should be in dependencies btw, not devDependencies as I discussed in that PR.

@BigForNothing I think you're right, as it's used by library code exposed to consumers like my project.

And oh, now I understand: I edited the wrong file; this is a monorepo and I should touch code in packages. Updating the PR, thanks for the poke.

@hasezoey hasezoey requested a review from nodkz January 22, 2020 21:26
@ronjouch
Copy link
Contributor Author

ronjouch commented Jan 22, 2020

@hasezoey @nodkz I pushed a new fix; ready for review.

Coming back to discussion in #265, I think @BigForNothing is right and @nodkz is missing the point. @nodkz , you say:

I always put @types/* packages to devDependencies. And I don't think that it's a good idea to install @types/* packages for a production build npm install --production.

To the best of my current knowledge, this is incorrect if you aim at being usable by strict: true TS projects (and I don't see why you wouldn't):

  1. The practical meaning of devDependencies is that they're not going to be installed if the packaged is installed transitively (as opposed to npm installing it). That's my case, I'm using mongoms in my project.
  2. Secondly, a strict: true project, everything must be typed for compilation to succeed.

Consequently, by putting the types in devDeps, they're not going to be installed in my strict: true TS project depending on mongoms, and compiling the tests (using mongoms) will fail. Currently, by chance, @types/tmp is the only missing package in my node_modules, but all your types for prod/lib code belongs to dependencies, not devDependencies, and in the new patch that's exactly what I'm suggesting: moving all prod/lib types to dependencies.

I stumbled on the same problem at work, and my policy is now:

  • types for prod code in deps, along with their respective package
  • types for test code in devDeps, along with their respective package.

For example, here's the project I'm working on; see how prod types are with prod libs in deps, and same for test types with test libs:

  "devDependencies": {
    "@types/jest": "24.x",
    "@types/supertest": "2.x",
    "@typescript-eslint/eslint-plugin": "2.x",
    "@typescript-eslint/parser": "2.x",
    "eslint": "6.x",
    "eslint-config-prettier": "6.x",
    "eslint-plugin-prettier": "3.x",
    "jest": "25.x",
    "jest-junit": "10.x",
    "mongodb-memory-server": "6.x",
    "npm-audit-resolver": "2.x",
    "prettier": "1.x",
    "supertest": "4.x",
    "typescript": "3.x"
  },
  "dependencies": {
    "@types/body-parser": "1.x",
    "@types/express": "4.x",
    "@types/mongoose": "5.x",
    "@types/node": "10.x",
    "@types/request-promise": "4.x",
    "body-parser": "1.x",
    "express": "4.x",
    "mongoose": "5.x",
    "request-promise": "4.x",
    "source-map-support": "0.x"
  }

Kinda sure about all that, but I welcome Daniel's take, maybe I'm missing something 🙂.

@hasezoey
Copy link
Member

@ronjouch i would like to not be involved in this discussion for this project, because i dont know what would be the right solution, and in its current state is the only thing i know and always done like it

anyway, my take was always something like "if it isnt included (/needed) after the compilation, dont put it in normal dependencies" (and it always worked, but i didnt have many projects in strict mode, because of the nature of these projects)

@ronjouch
Copy link
Contributor Author

ronjouch commented Jan 22, 2020

I would like to not be involved in this discussion for this project, because i dont know what would be the right solution, and in its current state is the only thing i know and always done like it

@hasezoey okay, leaving you out of it, and continuing the chat with @nodkz.

@nodkz , what I'm suggesting is that I did fiddle with that quite a bit at work, and I think I know the correct solution to satisfy strict: true projects, and I don't see a reason not to support those.

anyway, my take was always something like "if it isnt included (/needed) after the compilation, dont put it in normal dependencies" (and it always worked, but i didnt have many projects in strict mode, because of the nature of these projects)

Correct! The gotcha here is that, for strict: true TS projects, types are needed after compilation. So, as you say, they should be put in "normal dependencies".

Now, one may argue that strict: true TS projects are the exception rather than the rule. Maybe / I have no idea, but as a a happy TS user, I'm biased to disagree: the fact that mongoms uses tmp is none of my project's business, so I shouldn't have to add its types my package.json to compile my project depending on mongoms. To me, types belong to the build artifact delivered to library users, much like source maps, and much like C libs provide headers 🙂.

Does that makes sense / am I missing something? Thanks for mongoms.

@BigForNothing
Copy link
Contributor

@ronjouch Well said! I could be wrong, but I believe --isolatedModules require it too.

All of our projects at work require the strict: true setting, and I've never had a personal project without it.

@lirbank
Copy link

lirbank commented Jan 24, 2020

@types/tmp should be in dependencies (not devDependencies). See:
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#dependencies

@ronjouch ronjouch changed the title Add @types/tmp to devDeps Add @types/tmp to dependencies Jan 24, 2020
@nodkz nodkz merged commit ffbf1ea into typegoose:master Jan 27, 2020
@nodkz
Copy link
Collaborator

nodkz commented Jan 27, 2020

@ronjouch thanks for PR and the detailed explanation of strict mode in TS.
Merged! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion This Feature/enhancement still needs to be decided if it is wanted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants