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

optionalDependencies should be peerDependencies #341

Open
1 of 5 tasks
MatthiasKunnen opened this issue May 1, 2023 · 4 comments
Open
1 of 5 tasks

optionalDependencies should be peerDependencies #341

MatthiasKunnen opened this issue May 1, 2023 · 4 comments

Comments

@MatthiasKunnen
Copy link

I'm submitting a...

  • Regression
  • Bug report
  • Feature request
  • Documentation issue or request
  • Support request

Current behavior

Installing nest-typed-config installs the following optional dependencies:

  • @iarna/toml
  • @nestjs/axios
  • cosmiconfig
  • dotenv
  • dotenv-expand
  • parse-json
  • yaml

While it is documented to use npm install --no-optional, this is not supported in other package managers and is not the actual use case for optionalDependencies.

npm

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies object.
https://docs.npmjs.com/cli/v9/configuring-npm/package-json?v=true#optionaldependencies

yarn

Similar to the dependencies field, except that these entries will not be required to build properly should they have any build script. Note that such dependencies must always be resolvable (otherwise we couldn't store it in the lockfile, which could lead to non-reproducible installs), but those which list os / cpu / libc fields will not be fetched unless they match the current system architecture.

This field is usually not what you're looking for, unless you depend on the fsevents package. If you need a package to be required only when a specific feature is used then use an optional peer dependency. Your users will have to satisfy it should they use the feature, but it won't cause the build errors to be silently swallowed when the feature is needed.
https://yarnpkg.com/configuration/manifest#optionalDependencies

Expected behavior

The dependencies should instead be peerDependencies with peerDependenciesMeta setting optional to true.

What is the motivation / use case for changing the behavior?

Ideally, unused dependencies are not included when installing a package.

Environment


Nest version: 9.0.11
Nest-Typed-Config version: 2.5.2

 
For Tooling issues:
- Node version: v18.16.0
- Platform: Linux

Others:
Package manager: [email protected] and [email protected]

@Nikaple
Copy link
Owner

Nikaple commented May 4, 2023

Migrating optional dependencies to peer dependencies will require a major update, which is what I want to avoid for a stable package.

For example, when the application uses dotenvLoader but only has dependency of nest-typed-config, but not dotenv, there will be an runtime error: The "dotenv" package is missing. Please, make sure to install this library ($ npm install dotenv) to take advantage of dotenvLoader.

With yarn, you can try:

yarn install nest-typed-config --ignore-optional

@MatthiasKunnen
Copy link
Author

It will indeed be a breaking change.

Yarn does not have an --ignore-optional flag (see https://yarnpkg.com/cli/install) and neither does pnpm.

While I understand the aversion to breaking changes, unfortunately, the way this package is using optionalDependencies at this moment is not the way it was intended to be used. For this reason, package managers do not support the approach you are suggesting. See the Yarn docs I referenced:

This field is usually not what you're looking for, unless you depend on the fsevents package. If you need a package to be required only when a specific feature is used then use an optional peer dependency. Your users will have to satisfy it should they use the feature, but it won't cause the build errors to be silently swallowed when the feature is needed.

@Nikaple
Copy link
Owner

Nikaple commented May 5, 2023

I tried using Node.js version 18 and enabled yarn with the command corepack enable. I discovered that the default version of yarn, which is version 1, includes the --ignore-optional flag and works well. However, after upgrading to yarn@3, this flag is no longer available.

To resolve this issue, you have several options:

  1. Downgrade yarn to version 1.
  2. Fork the repository and release your own package.
  3. Alternatively, you can choose to disregard the issue as it's just some extra dependencies.

As a user, I dislike major updates and breaking changes, and I believe that others share this sentiment. Therefore, it is not feasible to implement a major update that only involves dependency changes.

@MatthiasKunnen
Copy link
Author

I understand your point of view.

Perhaps it can hitch along a potential future breaking change. Until that time, feel free to keep this issue open or close it.

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

No branches or pull requests

2 participants