nodemailer / wildduck

Opinionated email server
https://wildduck.email/
European Union Public License 1.2
1.91k stars 267 forks source link

Moving from Restify to Hapi #161

Closed rakshith-ravi closed 8 months ago

rakshith-ravi commented 4 years ago

The package joi seems to be depreciated according to it's npm page. The author recommends switching to @hapi/joi.

I'll put up a PR to fix this soon

andris9 commented 4 years ago

I already tried this some time ago, thought that it is just renaming package name but it turned out to be a bit more complex, breaking changes in the library, so I reverted. Problem is that a lot of API paths (this is where joi is used the most) do not have tests and thus the problems are not immediately found. Currently the most viable option is to install wildduck-webmail and then try to use all the features to see if anything breaks.

rakshith-ravi commented 4 years ago

Okay. Got it. I've put up a PR with the new changes. I'll test it out with the webmail and see if it works.

singingwolfboy commented 4 years ago

Ajv does the same basic thing that Joi does, and it does it in a more declarative and reusable way. How would you feel about migrating from Joi to Ajv, instead? I'm happy to write JSON schemas for all the places in the codebase that currently use Joi schemas.

Note that this may require minor differences in logic for the parsed result. For example, the current code has logic to coerce values from one type to another, so that the string "yes" is parsed as boolean true. Ajv does not support this kind of data modification: data is either valid or invalid, but is not modified in order to suit the schema. If we switch to Ajv, the logic will have to account for coercing "yes" to boolean true, for example.

rakshith-ravi commented 4 years ago

2 questions:

  1. Wouldn't that break existing API calls that depend on such data modification?
  2. Can't we modify the data before sending it to Ajv? So that we can convert such modifications manually and then send it over.

I haven't looked at the package, so I'm not sure how it works.

andris9 commented 4 years ago

I'd rather keep Joi and possibly migrate from Restify to Hapi that has built in Joi integration + automatic swagger documentation page generator. I did this with IMAP API and it worked very well.

singingwolfboy commented 4 years ago

Wouldn't that break existing API calls that depend on such data modification?

This should not break API calls, as long as we update the logic to account for backwards compatibility. For example, to convert code that looks like this:

// old way, using Joi
const result = Joi.validate(req.params, schema)
if (result.error) {
  // return error message
}
const showAll = result.value.showAll
console.log(showAll) // boolean

You can do this:

// new way, using Ajv
const validate = ajv.compile(schema);
const valid = validate(req.params)
if (!valid) {
  // return error message
}
const rawShowAll = req.params.showAll;
const showAll = typeof rawShowAll === "boolean" ? rawShowAll : ['Y', 'true', 'yes', 'on', '1'].includes(rawShowAll)
console.log(showAll) // boolean
andris9 commented 4 years ago

From AJV docs:

JSON Schema draft-07 also defines formats iri, iri-reference, idn-hostname and idn-email for URLs, hostnames and emails with international characters. Ajv does not implement these formats.

WildDuck supports IDN emails (that is unicode characters in email username parts) so if Ajv does not support it then there is no point in migrating.

louis-lau commented 4 years ago

Using hapi would also tackle #179. I've already started converting some apidoc to openapi in jsdoc, but it's a pain in the ass to be completely honest. Having automatically generated docs would be much nicer. Keeping in mind that you'd probably want to use something that supports openapi 3. Using TypeScript to auto generate it is even nicer. But probably too late for that now, or not preferable by everyone.

singingwolfboy commented 4 years ago

There is an Ajv plugin that provides support for IDN emails. However, if the developers are not interested in switching to Ajv, then it doesn't really matter.

github-actions[bot] commented 8 months ago

This issue is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 15 days.

github-actions[bot] commented 8 months ago

This issue was closed because it has been stalled for 15 days with no activity.