-
Notifications
You must be signed in to change notification settings - Fork 14
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
Created a 'generic' serialize method, useful to use with web sockets. #15
base: master
Are you sure you want to change the base?
Conversation
Thanks @oiramalli! I've been out of the game lately, so I can't give this a terribly thorough review. Could you take a quick look, @jelhan? If I don't hear anything from @jelhan in the next day or so, I'll go ahead and merge anyway. :). |
@oiramalli Thanks for your work. Would it be okay for you to add some tests? |
lib/responses/ok.js
Outdated
attributes: modelUtils.getAttributes(Model) | ||
attributes: modelUtils.getAttributes(Model), | ||
keyForAttribute:sails.config.jsonapi.keyForAttribute, | ||
pluralizeType:sails.config.jsonapi.pluralizeType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a space after colon.
|
Just cleaned what @jelhan pointed out. |
Just one thing... |
@IanVS I was also wondering why tests didn't cached that ones. @oiramalli Thanks for cleaning up. Naming of files in |
@IanVS I get that error in my application. |
@oiramalli Could you please rebase after #17 to have correct CI reports? I'm really sorry about that one. The maximum call size exceeded issue sound like an infinitive loop. json-api-serializer overrides |
Shouldn't need a rebase, I can restart the test. |
Strange, Travis should be merging this PR with latest master to run the tests. |
lib/hook.js
Outdated
addResponseMethods(req, res); | ||
normalizePayload(req, next); | ||
next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be a call to next
here. Has to wait for normalizePayload
and therefore that one should call next
. After removing this line (and ignoring still present coding stile issues) all tests passed for 8921274.
lib/serializer.js
Outdated
|
||
module.exports = function (modelName, data) { | ||
let sails = this.req._sails; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed. It's used several times. Causing your tests to fail.
…s-hook-jsonapi into generalSerializer
f74d978
to
f345004
Compare
@oiramalli Feel free to ask if you need any help. |
Hello, I just added a serialize method because I needed to send the same JsonAPI Spec through web sockets and integrated them with this library.
I think this will be very useful and expand this library functionality a little bit more.