Skip to content

Conversation

@orgads
Copy link

@orgads orgads commented Dec 3, 2024

These are dev dependencies.

These are dev dependencies.
Copy link
Owner

@davidje13 davidje13 left a comment

Choose a reason for hiding this comment

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

Since this project includes its own types, it is unfortunately necessary to mark these as peer dependencies.

  • using dependencies would make it possible for the types used by websocket-express and the types used by a consumer of the library to diverge (different versions), which would cause lots of difficult-to-debug issues
  • using devDependencies would cause the project to fail to build for consumers using TypeScript in their projects, with no obvious fix (and when using Yarn with isolated dependencies, no possible fix)
  • using peerDependencies with peerDependenciesMeta to mark them as optional fixes the issue of divergent versions, but still makes it difficult for consumers using TypeScript to know which dependencies they need
  • Moving the types to DefinitelyTyped as a separate package is not practical to maintain (and doing so would go against Microsoft's own recommendations)

In particular note that the practice of including @types dependencies as devDependencies is only relevant to applications; not libraries.

The disadavntage of the current approach, of course, is that this means users who don't use TypeScript have to either install unnecessary dependencies, or live with warnings about unmet peer dependencies. Fortunately, you only need to install these as devDependencies (which is the correct approach for an application) to meet the peerDependency requirement.

My recommendation is to include these as devDependencies in your own project. Even if you are not using TypeScript yourself, these packages can still be useful to IDEs which are often able to offer more useful assistance based on these definitions. They will not be part of your built application.

@orgads
Copy link
Author

orgads commented Dec 3, 2024

I already have them as devDependencies, but when I installed websocket-express the "dev": true part was removed from package-lock. I'll try to reproduce it tomorrow.

@orgads
Copy link
Author

orgads commented Dec 5, 2024

The disadvantage is not only for users who don't use TypeScript. It is also for any package that depends on websocket-express and installs dependencies with --omit=dev or with NODE_ENV=production. It means that types packages will needlessly be installed on the target image.

@orgads
Copy link
Author

orgads commented Dec 5, 2024

I see there is a stale RFC about this: npm/rfcs#553

@davidje13
Copy link
Owner

I'm happy to track the upstream issue and improve these dependencies if/when they add better support for this situation, but for now I think there simply isn't a perfect setup available, and keeping them as peerDependencies seems to be the least harmful option.

I could be convinced to mark them as optional if that's common practice in similar libraries and isn't too confusing for users. I don't know if that would help with your particular issue or not.


On your application you might want to look at using a bundler to combine your production dependencies into your distributable (removing the need for customers to explicitly install any dependencies).

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.

2 participants