Join GitHub today
Calling Serializer/Deserlizer without new crashes node. Adding a js class which just inherits cpp bindings. Fixes: #13326 Checklist make -j4 test (UNIX), or vcbuild test (Windows) passes (test failing on master too) tests and/or benchmarks are included documentation is changed or added commit message follows commit guidelines Affected core subsystem(s). I am interested in finding out what binary serialization frameworks/libraries are there for node.js. I tried to use Apache Thrift, but the documentation is very poor and does not offer any exampl.
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upHave 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
Conversation
commented Jun 8, 2017 • edited
edited
Calling Serializer/Deserlizer without new crashes node. Adding a js class which just inherits cpp bindings. : #13326 Checklist
Affected core subsystem(s) |
v8: add a js class for Serializer/Dserializer
commented Jun 8, 2017
Can you add some regression tests? There were some gotchas around new.target and native code but I forgot the details. It would be interesting to see if class S extends v8.Serializer works and what instance.method.call({}) does.I expect it throws an 'illegal invocation' exception but it would be good to verify that. |
approved these changes Jun 8, 2017
commented Jun 8, 2017
Php Vs Node Js
It does, we already extend it :) |
commented Jun 8, 2017
I mean when you extend the class that this PR introduces. |
commented Jun 8, 2017 • edited
edited
@bnoordhuis, the classes this PR introduces are already extended further down the file by DefaultSerializer and DefaultDeserializer . |
commented Jun 8, 2017
Okay. Regression tests for the other issues would still be good though. |
v8: add a js class for Serializer/Dserializer
commented Jun 8, 2017
@bnoordhuis : Added regression tests. |
requested changes Jun 8, 2017
@@ -131,3 +131,19 @@ const objects = [ | |
assert.deepStrictEqual(v8.deserialize(buf), expectedResult); | |
} | |
{ | |
try { |
Jun 8, 2017
Jun 8, 2017
v8: add a js class for Serializer/Dserializer
reviewed Jun 8, 2017
v8.Serializer(); |
}, |
Error, |
'Class constructor Serializer cannot be invoked without 'new'' |
Jun 8, 2017
Can you do something more like this:
Jun 8, 2017
force-pushed the zimbabao:fix-serlializer-crash branch Jun 8, 2017
Node Js Commands
approved these changes Jun 8, 2017
commented Jun 8, 2017
There are typos in the commit message(s): s/Dserializer/Deserializer/ |
force-pushed the zimbabao:fix-serlializer-crash branch Jun 8, 2017
commented Jun 8, 2017
@mscdex : Thanks, fixed |
commented Jun 8, 2017
Can somebody fire a CI job for this. |
commented Jun 8, 2017 • edited
edited
s/refression/regression/ in commit message(s) too. CI: https://ci.nodejs.org/job/node-test-pull-request/8573/ |
v8: add a js class for Serializer/Deserializer
force-pushed the zimbabao:fix-serlializer-crash branch to 9643c6c
Jun 8, 2017
commented Jun 8, 2017
@mscdex : Fixed on top review. |
commented Jun 9, 2017
Nodejs With Php
CI again: https://ci.nodejs.org/job/node-test-pull-request/8574/ |
approved these changes Jun 9, 2017
left a comment
Still LGTM |
reviewed Jun 9, 2017
const { copy } =process.binding('buffer'); |
const { objectToString } =require('internal/util'); |
const { FastBuffer } =require('internal/buffer'); |
classSerializerextendsserdesBindings.Serializer {} |
classDeserializerextendsserdesBindings.Deserializer {} |
Jun 9, 2017
I'd prefer something like:
Jun 9, 2017
Jun 9, 2017
reviewed Jun 9, 2017
() => { v8.Deserializer(); }, |
/^TypeError: Class constructor Deserializer cannot be invoked without 'new'$/ |
); |
} |
Jun 9, 2017
Should likely also add a note to the documentation clarifying the requirement
v8: add a js class for Serializer/Deserializer
force-pushed the zimbabao:fix-serlializer-crash branch to 955a1bb
Jun 9, 2017
reviewed Jun 10, 2017
@@ -15,11 +15,21 @@ | |
'use strict'; | |
const { Buffer } =require('buffer'); | |
const { Serializer, Deserializer } =process.binding('serdes'); | |
const { | |
Serializer:_Serializer, |
Jun 10, 2017
Jun 10, 2017
Done. BTW jslint didn't catch that error. May be we should add a rule?
v8: add a js class for Serializer/Deserializer
commented Jun 10, 2017
Another CI run: https://ci.nodejs.org/job/node-test-pull-request/8588/ |
commented Jun 10, 2017
Tests pass on my mac osx sierra laptop. Above failures looks related to CI, can somebody start another run?. |
commented Jun 10, 2017
Yea, CI is a bit problematic right now, I’m not sure another run would solve much. This code isn’t platform-specific, though, so I think this is okay. |
commented Jun 10, 2017
@addaleax : Thanks, anything else for landing this?. |
commented Jun 10, 2017
@zimbabao Our rules say that we’ll need to wait until tomorrow to land this, but nothing from your side I think. :) |
approved these changes Jun 10, 2017
left a comment
LGTM % docs |
commented Jun 10, 2017
arm: #13603 osx: #13527 linux: infrastructure fail https://ci.nodejs.org/job/node-test-commit-linux/10495/nodes=ubuntu1604_docker_alpine34-64/ |
commented Jun 12, 2017
Landed in 12fd63d, thanks for the PR! |
added a commit that referenced this pull request Jun 12, 2017
v8: add a js class for Serializer/Dserializer
This commit was signed with a verified signature.
GPG key ID: D8B9F5AEAE84E4CFLearn about signing commits
added a commit that referenced this pull request Jun 12, 2017
v8: add a js class for Serializer/Dserializer
This commit was signed with a verified signature.
GPG key ID: D8B9F5AEAE84E4CFLearn about signing commits
Merged
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.
Active4 years, 6 months ago
I am wondering how can I parse Array of JSON objects in NodeJS?
Node Js Php Serialize
I want to post JSON array to the server, and be able to use the received array as a regualar JavaScript array.
Thanks in advance.
This is my front-end part that I am converting Array to String using stringify function
This my back-end part that I am trying to convert Array of JSON object to Array
This is Array that I am trying to sent to the server: How to install phpprobid mods.
AphaApha3601 gold badge9 silver badges28 bronze badges
3 Answers
In your
app.js
: Then you can just use
req.body
to get the posted values: In front-end, don't do any stringifying:
Vsevolod GolovizninVsevolod Goloviznin10k1 gold badge30 silver badges41 bronze badges
I'll try to explain this. First of all, you are crating a json string on the client.
Then on the server, you are doing the same again:
Then you parse the double stringifyed json string to a javascript object:
So now you actually have to parse it twice :). If you just stringify it on the server as you are now, and just call:
You will get a javascript object that you can access like this:
Have a look at this jsFiddle and open your developer tools. Look at the console when it runs and you will see where the problem is: example
cfscfs
You may actually send the JSON directly to server.
And in node.js, use
bodyParser.json
to get it back.By the way, do not use
Array
as variable name because Array
represent the Array
class used by JavaScript and your code has overwritten it.Alex LauAlex Lau