Skip to content

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

Open
oiramalli wants to merge 35 commits into
sane:masterfrom
oiramalli:generalSerializer
Open

Created a 'generic' serialize method, useful to use with web sockets.#15
oiramalli wants to merge 35 commits into
sane:masterfrom
oiramalli:generalSerializer

Conversation

@oiramalli

Copy link
Copy Markdown

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

IanVS commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

jelhan commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

@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.

Comment thread lib/responses/ok.js Outdated
attributes: modelUtils.getAttributes(Model)
attributes: modelUtils.getAttributes(Model),
keyForAttribute:sails.config.jsonapi.keyForAttribute,
pluralizeType:sails.config.jsonapi.pluralizeType

Copy link
Copy Markdown
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

IanVS commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Author

Just cleaned what @jelhan pointed out.

@oiramalli

Copy link
Copy Markdown
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

jelhan commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

@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

jelhan commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@oiramalli

Copy link
Copy Markdown
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

jelhan commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

@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

IanVS commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@jelhan

jelhan commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

@IanVS

IanVS commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

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

oiramalli commented Jul 25, 2016

Copy link
Copy Markdown
Author

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.

Comment thread lib/hook.js
if (req.isSocket) return next();
addResponseMethods(req, res);
normalizePayload(req, next);
next();

Copy link
Copy Markdown
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.

Comment thread lib/serializer.js Outdated
var JSONAPISerializer = require('jsonapi-serializer').Serializer;

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

Copy link
Copy Markdown
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

jelhan commented Feb 8, 2017

Copy link
Copy Markdown
Contributor

@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