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

oiramalli · 2016-07-21T00:27:15Z

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 · 2016-07-22T02:49:20Z

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 · 2016-07-22T11:45:26Z

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

jelhan · 2016-07-22T18:07:52Z

lib/responses/ok.js

-    attributes: modelUtils.getAttributes(Model)
+    attributes: modelUtils.getAttributes(Model),
+    keyForAttribute:sails.config.jsonapi.keyForAttribute,
+    pluralizeType:sails.config.jsonapi.pluralizeType


Please add a space after colon.

IanVS · 2016-07-22T18:16:38Z

~~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. ;)

…s-hook-jsonapi into generalSerializer

oiramalli · 2016-07-22T18:30:40Z

Just cleaned what @jelhan pointed out.

oiramalli · 2016-07-22T18:42:26Z

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 · 2016-07-22T19:44:05Z

@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 · 2016-07-22T19:55:43Z

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

oiramalli · 2016-07-22T20:00:37Z

@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 · 2016-07-22T20:20:29Z

@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 · 2016-07-22T20:21:41Z

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

jelhan · 2016-07-22T20:23:59Z

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

IanVS · 2016-07-22T20:25:57Z

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

…s-hook-jsonapi into generalSerializer

oiramalli · 2016-07-25T19:28:36Z

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.

I can't figure out what is wrong.

jelhan · 2016-07-26T08:43:00Z

lib/hook.js

+          if (req.isSocket) return next();
          addResponseMethods(req, res);
          normalizePayload(req, next);
+          next();


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.

jelhan · 2016-07-26T10:23:01Z

lib/serializer.js

+var JSONAPISerializer = require('jsonapi-serializer').Serializer;

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


This should not be removed. It's used several times. Causing your tests to fail.

…s-hook-jsonapi into generalSerializer

jelhan · 2017-02-08T11:07:45Z

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

oiramalli added 3 commits July 20, 2016 17:12

Created a serialaze 'generic' method.

8abff87

fix to to override the input on the config/jsonapi.js file

1e68a3a

fix for web sockets not going

610d202

jelhan reviewed Jul 22, 2016
View reviewed changes

oiramalli added 5 commits July 22, 2016 11:18

Created a serialaze 'generic' method.

23bfdc0

fix to to override the input on the config/jsonapi.js file

6065732

fix for web sockets not going

18dc298

Merge branch 'generalSerializer' of https://github.com/oiramalli/sail…

d3e7317

…s-hook-jsonapi into generalSerializer

Cleaning up files

a832828

Some more cleanup

f911673

oiramalli added 5 commits July 22, 2016 13:43

Created a serialaze 'generic' method.

670e70a

fix to to override the input on the config/jsonapi.js file

a39ca95

fix for web sockets not going

66a7511

Created a serialaze 'generic' method.

aafc544

fix to to override the input on the config/jsonapi.js file

74a11e7

oiramalli added 3 commits July 22, 2016 13:44

Cleaning up files

ebc65e5

Some more cleanup

00a9216

Merge branch 'generalSerializer' of https://github.com/oiramalli/sail…

17c219f

…s-hook-jsonapi into generalSerializer

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

oiramalli added 3 commits July 22, 2016 14:39

Some more cleanup...

39502f3

Some more cleanup...

310fd23

Some more cleanup...

8921274

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

jelhan reviewed Jul 26, 2016
View reviewed changes

wait for normalizePayload to call next()

18dd697

jelhan reviewed Jul 26, 2016
View reviewed changes

oiramalli added 14 commits December 8, 2016 12:21

Serializer

00fd1fa

Code cleanup

4e5dafd

Some changes on serializer

39acb87

Created a serialaze 'generic' method.

c674306

fix to to override the input on the config/jsonapi.js file

fb203e0

fix for web sockets not going

30c2afc

Created a serialaze 'generic' method.

af50ac4

fix to to override the input on the config/jsonapi.js file

a63972d

Cleaning up files

dc32182

Some more cleanup

81e9997

Created a serialaze 'generic' method.

b7c6ef6

fix to to override the input on the config/jsonapi.js file

15d00b3

Merge branch 'generalSerializer' of https://github.com/oiramalli/sail…

acbdc08

…s-hook-jsonapi into generalSerializer

Removing extra spaces

f345004

oiramalli force-pushed the generalSerializer branch from f74d978 to f345004 Compare February 8, 2017 08:36

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

oiramalli commented Jul 21, 2016

Uh oh!

IanVS commented Jul 22, 2016

Uh oh!

jelhan commented Jul 22, 2016

Uh oh!

jelhan Jul 22, 2016

Choose a reason for hiding this comment

Uh oh!

IanVS commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oiramalli commented Jul 22, 2016

Uh oh!

oiramalli commented Jul 22, 2016

Uh oh!

jelhan commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelhan commented Jul 22, 2016

Uh oh!

oiramalli commented Jul 22, 2016

Uh oh!

jelhan commented Jul 22, 2016

Uh oh!

IanVS commented Jul 22, 2016

Uh oh!

jelhan commented Jul 22, 2016

Uh oh!

IanVS commented Jul 22, 2016

Uh oh!

oiramalli commented Jul 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelhan Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

jelhan Jul 26, 2016

Choose a reason for hiding this comment

Uh oh!

jelhan commented Feb 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IanVS commented Jul 22, 2016 •

edited

Loading

jelhan commented Jul 22, 2016 •

edited

Loading

oiramalli commented Jul 25, 2016 •

edited

Loading