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

Created a 'generic' serialize method, useful to use with web sockets. #15

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

oiramalli
Copy link

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.

@IanVS
Copy link
Contributor

IanVS commented Jul 22, 2016

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. :).

@jelhan
Copy link
Contributor

jelhan commented Jul 22, 2016

@oiramalli Thanks for your work. Would it be okay for you to add some tests?

After merging #12 and #13 a rebase seems to be necessary.

attributes: modelUtils.getAttributes(Model)
attributes: modelUtils.getAttributes(Model),
keyForAttribute:sails.config.jsonapi.keyForAttribute,
pluralizeType:sails.config.jsonapi.pluralizeType
Copy link
Contributor

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.

@IanVS
Copy link
Contributor

IanVS commented Jul 22, 2016

Hm, this is on us for not having a linting step in our ci. I guess we do, not sure why it didn't catch these, but I'll look into it later. I'm happy to clean up styles, @oiramalli if you don't have the time or patience for it. ;)

@oiramalli
Copy link
Author

Just cleaned what @jelhan pointed out.

@oiramalli
Copy link
Author

Just one thing...
On master you have /lib/responses/json.js; if I try to run it like this, I get a Maximum call stack size exceeded
But everything works fine if I change the name /lib/responses/ok.js
I'm I doing something wrong?? or is it normal??

@jelhan
Copy link
Contributor

jelhan commented Jul 22, 2016

@IanVS I was also wondering why tests didn't cached that ones.

@oiramalli Thanks for cleaning up. Naming of files in /lib/responses/ makes a difference cause /lib/hooks.js binds them to response object. Filename was changed in 7b29471 so it could be also used for non 200 OK responses (like 201 Created). Do you get the call size exceeded when running tests or in your consuming application?

@jelhan
Copy link
Contributor

jelhan commented Jul 22, 2016

@IanVS CI didn't reported coding stile issues cause travis didn't run any test. Have a look at #16.

@oiramalli
Copy link
Author

@IanVS I get that error in my application.
I started using the npm package of this library, then noticed that it was not up to date, so I pulled the master branch from github and added it to my node modules.
Not sure but I think i was this 7884e99 the one I pulled and forked, but i had to change the name back to /lib/ok.js for it to work with my application.

@jelhan
Copy link
Contributor

jelhan commented Jul 22, 2016

@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 json method of response object.

@IanVS
Copy link
Contributor

IanVS commented Jul 22, 2016

Shouldn't need a rebase, I can restart the test.

@jelhan
Copy link
Contributor

jelhan commented Jul 22, 2016

@IanVS I tried the same but since .travis.yml is broken before #17 tests won't run before rebase.

@IanVS
Copy link
Contributor

IanVS commented Jul 22, 2016

Strange, Travis should be merging this PR with latest master to run the tests.

@jelhan jelhan changed the title Created a 'generic' serialaze method, useful to use with web sockets. Created a 'generic' serialuze method, useful to use with web sockets. Jul 22, 2016
@jelhan jelhan changed the title Created a 'generic' serialuze method, useful to use with web sockets. Created a 'generic' serialize method, useful to use with web sockets. Jul 25, 2016
@oiramalli
Copy link
Author

oiramalli commented Jul 25, 2016

Hey guys, IDK if you could walk me trough the tests because those are failing on my computer.

npm version: 3.10.6
node version: 6.3.0
sails version: 0.12.3

When I try to run npm test I get:

  total:     38
  passing:   9
  failing:   29
  duration:  3.5s

And it shows this error on most of the failed tests.
captura de pantalla 2016-07-25 a las 12 26 03 p m

I can't figure out what is wrong.

lib/hook.js Outdated
addResponseMethods(req, res);
normalizePayload(req, next);
next();
Copy link
Contributor

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.


module.exports = function (modelName, data) {
let sails = this.req._sails;
Copy link
Contributor

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.

@jelhan
Copy link
Contributor

jelhan commented Feb 8, 2017

@oiramalli Feel free to ask if you need any help.

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

Successfully merging this pull request may close these issues.

3 participants