Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Deprecate and kill async API #68

Closed
XVincentX opened this issue Aug 3, 2016 · 3 comments · Fixed by #178
Closed

Deprecate and kill async API #68

XVincentX opened this issue Aug 3, 2016 · 3 comments · Fixed by #178

Comments

@XVincentX
Copy link
Contributor

According to the documentation, gavel.js offers an async API to perform validation and other operations.

According to the code, this is not an async API but a wrapper that is blocking the event loop anyway.

This is bad and harmful as people thinks that the validation is non blocking (so I do not get worried about giant payloads as is off process), while at the end of the day it is.

Please deprecate and kill this API as soon as possible.

@artem-zakharchenko
Copy link
Contributor

artem-zakharchenko commented May 16, 2019

I would deprecate both current sync and async API. After #155 lands, validation of a given transaction is a function that returns a validation result. There is no need to keep an overhead of classes, and thus the following API becomes cumbersome and obsolete:

const { HttpRequest, ExpectedHttpRequest } = require('gavel')

const request = new HttpRequest({ ... })
request.expected = new ExpectedHttpRequest({ ... })
request.isValid()

When you can do:

const { validate } = require('gavel')

const request = { ... }
const expectedResult = { ... }
const result = validate(request, expectedRequest)

@artem-zakharchenko
Copy link
Contributor

artem-zakharchenko commented May 24, 2019

Reading through isValidatable implementation I find it extremely over-engineered and complex. It gives a wrong expectation to the end user what the method actually does. Here's the usage example from the tests:

before((done) => {
  isValidatable(
    similarHttpMessage,
    baseHttpMessage,
    variant,
    (err, result) => {
      error = err;
      results = result;
      done();
    }
  );

One would assume the call signature of this function is something like this:

type IsValidatable = (httpMessage: any, anotherHttpMessage:any, type: string, callback: Function) => void

But it's not.

Current isValidatable operates only on the first argument. We must rework this method entirely, and start from understanding what purpose and usage it has.

@ApiaryBot
Copy link
Collaborator

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

4 participants