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): Adds express/fastify to peer dependency list and makes them optional #1008

Merged

Conversation

kamronbatman
Copy link
Contributor

@kamronbatman kamronbatman commented Oct 7, 2022

Adds express/fastify to the peer dependency list to fix a problem with yarn pnp workspaces and resolving dependencies from @nestjs/common through @nestjs/serve-static.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

fix: Fixes a Nestjs runtime error saying it cannot find express when using yarn pnp workspaces.

  • 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?

Receive the error: "The "express" package is missing. Please, make sure to install this library ($ npm install express) to take advantage of ServeStaticModule."

Screen Shot 2022-10-07 at 1 53 13 PM

Issue Number: N/A

What is the new behavior?

The ServeStaticModule loads properly and this error does not show up.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Adds express to peer dependency to fix a problem with yarn pnp workspaces and resolving dependencies from @nestjs/common through @nestjs/serve-static.
@kamronbatman
Copy link
Contributor Author

@kamilmysliwiec, since this is my first PR, I am not sure if I filled it out correctly. Let me know if I missed anything.

package.json Outdated
@@ -19,7 +19,8 @@
},
"peerDependencies": {
"@nestjs/common": "^9.0.0",
"@nestjs/core": "^9.0.0"
"@nestjs/core": "^9.0.0",
"express": "^4.8.1"
Copy link
Member

@micalevisk micalevisk Oct 7, 2022

Choose a reason for hiding this comment

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

express is optional here because this package could be used with fastiy.

You could add fastify as well and mark both of them as optional with https://docs.npmjs.com/cli/v8/configuring-npm/package-json#peerdependenciesmeta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea. I'll do that and test it out with yarn pnp workspaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@micalevisk I updated with the following:

  • I had the wrong express version, so fixed that.
  • Added fastify and fastify/static.
  • Added all 3 to peerDependenciesMeta so they are optional

As far as I can tell it is working well when I consume it using yarn v3.2.4 + pnp.

@kamronbatman kamronbatman changed the title fix(core): Adds express to peer dependency list fix(core): Adds express/fastify to peer dependency list and makes them optional Oct 10, 2022
@kamilmysliwiec kamilmysliwiec merged commit dc96f7d into nestjs:master Feb 9, 2023
@kamilmysliwiec
Copy link
Member

lgtm

@kamronbatman kamronbatman deleted the kbatman/yarn_pnp_workspace branch February 10, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants