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

Remove joi validation when creating request podium events #3532

Closed
hueniverse opened this issue Jun 29, 2017 · 6 comments
Closed

Remove joi validation when creating request podium events #3532

hueniverse opened this issue Jun 29, 2017 · 6 comments
Assignees
Labels
Milestone

Comments

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jun 29, 2017

https://medium.com/walmartlabs/optimizing-hapijs-for-benchmarks-737f371265e9

@hueniverse hueniverse added the bug label Jun 29, 2017
@hueniverse hueniverse self-assigned this Jun 29, 2017
@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 29, 2017

If you can give a slight pointer on what needs to be removed this would very nice for newcomer to PR, what do you think?

@wswoodruff
Copy link
Contributor

@wswoodruff wswoodruff commented Jun 29, 2017

I'd give it a shot!

@devinivy
Copy link
Member

@devinivy devinivy commented Jun 29, 2017

While that would be awesome, I expect this to require changes to both podium and hapi, which might make it more complex than a typical new contributor issue. What changes those should be is also somewhat of a design decision. Does someone see a simpler path?

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jun 29, 2017

@devinivy oh I thought it was just removing some joi validation calls?

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Jun 29, 2017

We can still validate the event but not validate the same events over and over.

I was thinking in something like this:

const finishEvent = new Podium.Event('finish'); //event was validated here

somePodiumInstance.registerEvent(finishEvent); //no need to validate since it is a Podium.Event instance.

somePodiumInstance.registerEvent('disconnect'); //validate this one as we already do

in request.js

internals.events = {
    finish: new Podium.Event('finish'),
    peek: new Podium.Event({ name: 'peek', spread: true }),
    disconnect: new Podium.Event('disconnect')
}

/*...*/

internals.Request = function (connection, req, res, options) {

    Podium.call(this, internals.events.finish, internals.events.peek, internals.events.disconnect);

    /*...*/

If everyone like this approach I can start by making a PR to podium to get this moving :P

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Jun 29, 2017

Just leave it for me... hence why it is already assigned. I am still thinking how I want to implement this.

@hueniverse hueniverse added this to the 16.5.0 milestone Jul 18, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants