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

Changed unknown event logic #24

Merged
merged 3 commits into from Dec 19, 2014

Conversation

@arb
Copy link
Contributor

arb commented Dec 16, 2014

Closes #23
Closes #25

Because we can't really know the structure of these events, we really can't safely do anything with them when we try to display the result.

This look ok @lloydbenson?

@lloydbenson

This comment has been minimized.

Copy link
Contributor

lloydbenson commented Dec 17, 2014

overall seems fine. I see we only have one case left for safe stringify might be nice to get rid of that module if its not required. I have asked @geek to take a look too.

@geek geek added this to the 3.1.0 milestone Dec 17, 2014
var responsePayload = '';
var statusCode = '';

if (typeof event.responsePayload === 'object' && event.responsePayload) {
responsePayload = 'response payload: ' + SafeStringify(event.responsePayload);
responsePayload = 'response payload: ' + JSON.stringify(event.responsePayload);

This comment has been minimized.

Copy link
@geek

geek Dec 17, 2014

Member

Is this safe?

This comment has been minimized.

Copy link
@arb

arb Dec 17, 2014

Author Contributor

I think so... only way this would blow up is if the responsePayload has circular objects... I guess nothing (in Hapi) really prevents that from happening does it?

This comment has been minimized.

Copy link
@geek

geek Dec 17, 2014

Member

right, I think we will need safe stringify for this line

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 17, 2014

Not sure I understand the point, what does this buy us?

@arb

This comment has been minimized.

Copy link
Contributor Author

arb commented Dec 17, 2014

If any event comes in we don't know what it is, it will just log a warning about it instead of guessing at the structure and requiring a bunch of checks and ||. We can also remove SafeStringify as well since nothing going into printResponse is unknown.

@geek

This comment has been minimized.

Copy link
Member

geek commented Dec 17, 2014

Is this really a breaking change?

@arb

This comment has been minimized.

Copy link
Contributor Author

arb commented Dec 17, 2014

If you are relying on that else block printing out generic events, yeah probably. I'd be ok with just bumping minor though if you are.

@arb arb force-pushed the arb:unknown-event branch from cac2612 to cd3e21f Dec 17, 2014
@arb arb removed the breaking changes label Dec 17, 2014
@arb arb assigned cjihrig and unassigned geek Dec 19, 2014

console.log(output);
};

internals.printResponse = function (event, format) {

var query = event.query ? SafeStringify(event.query) : '';

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 19, 2014

Contributor

I noticed that @lloydbenson said this was the last use of SafeStringify(). Does that mean package.json should be updated as well?

This comment has been minimized.

Copy link
@arb

arb Dec 19, 2014

Author Contributor

No we need it somewhere else in the file.

@arb arb force-pushed the arb:unknown-event branch from 12fe210 to 6194386 Dec 19, 2014
cjihrig added a commit that referenced this pull request Dec 19, 2014
Changed unknown event logic
@cjihrig cjihrig merged commit e999d12 into hapijs:master Dec 19, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@arb arb deleted the arb:unknown-event branch Dec 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.