Skip to content
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

fix(core): add missing optional dependencies #5477

Merged
merged 1 commit into from Dec 9, 2020

Conversation

andreialecu
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

@nestjs/platform-express is hardcoded as the default platform, and is required in various places in the code base. However, it is not an explicit dependency and thus package installers (yarn/npm/pnpm) need a hint on how to properly link them in order to be resolved.

See:

private createHttpAdapter<T = any>(httpServer?: T): AbstractHttpAdapter {
const { ExpressAdapter } = loadAdapter(
'@nestjs/platform-express',
'HTTP',
() => require('@nestjs/platform-express'),
);
return new ExpressAdapter(httpServer);
}

Without this change, Nest has problems running on stricter package managers (yarn with pnp, or pnpm)

What is the new behavior?

@nestjs/platform-express is added as an optional dependency. peerDependenciesMeta is supported by all major package managers: npm, yarn and pnpm.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

See: https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

@coveralls
Copy link

coveralls commented Sep 25, 2020

Pull Request Test Coverage Report for Build b6459367-ea49-40fc-b5bb-8ffd4337ae59

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.825%

Totals Coverage Status
Change from base Build cf4f5692-9ee1-4371-8130-5c6f9c02faac: 0.0%
Covered Lines: 4929
Relevant Lines: 5198

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

We should probably add @nestjs/microservices and @nestjs/websockets as well as they are referenced in the codebase

@andreialecu
Copy link
Contributor Author

@kamilmysliwiec shall I add them as part of this same PR?

@kamilmysliwiec
Copy link
Member

@andreialecu sounds good!

@andreialecu andreialecu changed the title fix(core): add optional dependency on @nestjs/platform-express fix(core): add missing optional dependencies Sep 28, 2020
@andreialecu
Copy link
Contributor Author

Added.

@andreialecu
Copy link
Contributor Author

andreialecu commented Oct 19, 2020

It seems that class-validator and class-transformer may also need to be added:

classValidator = loadPackage('class-validator', 'ValidationPipe', () =>
require('class-validator'),
);
classTransformer = loadPackage('class-transformer', 'ValidationPipe', () =>
require('class-transformer'),
);

Originally reported at: nestjs/nest-cli#555 (comment)

Probably everything else that comes up in other calls to loadPackage(...) may need to go through the same treatment as well.

@andreialecu
Copy link
Contributor Author

It might worth it to explore whether this eslint rule can be implemented:
https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-extraneous-dependencies.md

@kamilmysliwiec
Copy link
Member

@andreialecu

Probably everything else that comes up in other calls to loadPackage(...) may need to go through the same treatment as well.

Would you like to do it in this PR?

@andreialecu
Copy link
Contributor Author

andreialecu commented Oct 29, 2020

@kamilmysliwiec done.

I used npx @yarnpkg/doctor . to find them all (see: https://yarnpkg.com/getting-started/migration#run-the-doctor)

There are still a bunch of warnings left, but they're internal to the project (dev dependencies, etc) so I didn't mess with those (since they won't affect end users of packages). They can be fixed as part of a subsequent PR.

@kamilmysliwiec kamilmysliwiec removed this from the 7.5.0 milestone Oct 30, 2020
@andreialecu
Copy link
Contributor Author

@kamilmysliwiec just some further info:

All recent package managers support optional peer dependencies using the peerDependenciesMeta field:

npm as of: npm/cli#224
Yarn 1: https://classic.yarnpkg.com/en/docs/package-json#toc-peerdependenciesmeta
Yarn 2: https://yarnpkg.com/configuration/manifest/#peerDependenciesMeta
pnpm: https://pnpm.js.org/en/package_json#peerdependenciesmeta

@@ -22,6 +22,7 @@
"peerDependencies": {
"@nestjs/common": "^7.0.0",
"@nestjs/core": "^7.0.0",
"rxjs": "^6.0.0"
"rxjs": "^6.0.0",
"reflect-metadata": "^0.1.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is actually not optional, see:

import 'reflect-metadata';

@@ -26,8 +26,47 @@
"@nestjs/core": "7.4.4"
},
"peerDependencies": {
"reflect-metadata": "^0.1.12",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not optional, see:

import 'reflect-metadata';

@andreialecu
Copy link
Contributor Author

Also relevant: moleculerjs/moleculer#623 (comment)

@andreialecu
Copy link
Contributor Author

@kamilmysliwiec is there anything holding this from being merged? There have been a bunch of support requests in the Yarn discord related to NestJS not working properly out of the box and this is among the last things needed.

@stylemistake
Copy link

stylemistake commented Dec 2, 2020

We use yarn berry with workspaces, but without the monorepo flag set in nest-cli. If using typescript compiler, it works even without that huge of a list of peer dependency overrides, but it becomes a massive problem with webpack flag set.

Some peer dependencies that might be missing from this PR:

  "@nestjs/mapped-types@*":
    peerDependenciesMeta:
      "class-transformer":
        optional: true
      "class-validator":
        optional: true
  "@nestjs/serve-static@*":
    peerDependenciesMeta:
      "express":
        optional: true
  "@nestjs/swagger@*":
    peerDependenciesMeta:
      "swagger-ui-express":
        optional: true

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

Successfully merging this pull request may close these issues.

None yet

4 participants