search

arb / celebrate

A joi validation middleware for Express.
MIT License
1.34k stars 65 forks source link

@types/hapi__joi is not installed by default #171

Closed isaachinman closed 4 years ago

isaachinman commented 4 years ago

Hi there. Would you consider installing @types/hapi__joi by default, to support TypeScript users? Took me a few minutes to track the types down. If you just install celebrate and attempt to use the Joi export, it will be typed as any until you install @types/hapi__joi. The @types/hapi__joi package should be a dependency of celebrate.

Happy to submit a PR.

arb commented 4 years ago

Probably not, no. I add all the TypeScript overhead to make working with celebrate easier and provide a friendly developer experience in IDEs that support it; but @types/hapi__joi is not required for celebrate to run and I'd like to only add things to the dependencies that are required for the code to run.

isaachinman commented 4 years ago

Well, you're expecting users to use the Joi export, so I'd expect types alongside that.

To put it another way: type annotations in general are not required for celebrate to run, but every single TS project who installs celebrate will also have to track things down and eventually install @types/hapi__joi.

This would be a different story if @hapi/joi was a peerDependency and you expected it to be managed in user land. But you're pulling it in here as a regular dep and re-exporting it to users.

amitkirdatt commented 4 years ago

Hey @isaachinman it looks @types/hapi__joi is installed as a devDependency here. I am curious why that doesn't address the issue?

isaachinman commented 4 years ago

@amitkirdatt devDependencies are exactly that... They are not installed during prod installs.

amitkirdatt commented 4 years ago

Can you talk a bit about what the usecase is for using it in prod?

isaachinman commented 4 years ago

@amitkirdatt By "prod", we mean when the package is installed.

remyoudemans commented 4 years ago

From this line in the type declaration file, it's clear that at least the person who wrote that line agreed that celebrate should provide the types for Joi as well as its functionality.

However, since @types/hapi__joi is in the devDependencies, the Joi types imported in the type declaration file aren't present in the final bundled package.

Since celebrate bothers having to have a type declaration file, it makes sense that it would actually provide all the types for the API it exposes.

remyoudemans commented 4 years ago

@arb that being said, if you can't be swayed to add it to the dependencies, it would be good to at least add a line in the README on how to install those typings, specifying the appropriate version.

arb commented 4 years ago

@isaachinman So I'm reviewing this again and I'm still a bit confused. There are a bunch of ways to get npm or yarn to install dev deps if they are not installed by default. This varies depending on the version of npm or yarn you're using, but it's well documented. I'm not clear the situation you are in when the type definition for Joi is causing your code not to function correctly. Can you expand on that?

As previously state, the type defs were added to improve developer experience (DX) only; not to make celebrate type sound in TypeScript. This is why i only include it as a dev dep; you only get the DX benefit when actively doing development . This is not a TypeScript module and the existence of definition files or not does not prevent the code from executing as expected in Node as the unit and integration tests demonstrate.

I guess my final question is - what exactly is wrong, in your opinion, about not including the type defs in the regular dependencies? Does it prevent something from working? Or is this an issue where we disagree about what is and what is not "required" for the code to work as expected?

@remyoudemans I'm always open to documentation PRs! But again, this really boils down to the version of npm or yarn being used and the specifics of those package managers and how you tell them to also install dev deps.

arb commented 4 years ago

Also, since this has become the type def thread, I opened https://github.com/arb/celebrate/issues/181 if anyone is interested in contributing!

isaachinman commented 4 years ago

what exactly is wrong, in your opinion, about not including the type defs in the regular dependencies? Does it prevent something from working?

It's an easy enough inclusion that prevents every single TS user from having to track this issue down and figure out where to get types from. It is industry standard now for packages themselves to provide types on the stuff they export.

arb commented 4 years ago

Ok, you've changed my mind. I'll take a PR for this change and will also close #181.

isaachinman commented 4 years ago

Opened #182 – @remyoudemans can you please review? I no longer use this package in any projects.

@arb Side note, not having a lockfile can break semver.

remyoudemans commented 4 years ago

@isaachinman Looks good to me!