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

refactor(core-utils): use JSON.parse instead of fast-json-parse #3943

Closed
wants to merge 2 commits into from

Conversation

faustbrian
Copy link
Contributor

@faustbrian faustbrian commented Aug 9, 2020

Removes the fast-json-parse module because it's no longer necessary.

@@ -91,7 +94,7 @@ class Httpie {
const { body, headers, statusCode } = await got[method](url, opts);

return {
body: parseJSON(body).value,
body: JSON.parse(body || {}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: JSON.parse(body || {}),
body: JSON.parse(body || "{}"),

JSON.parse expects a string, not an object, otherwise you'll get a SyntaxError as it will try to parse [object Object] from {}.toString().

Object.defineProperty(this, "response", {
enumerable: false,
value: {
body: JSON.parse(error.response.body || {}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: JSON.parse(error.response.body || {}),
body: JSON.parse(error.response.body || "{}"),

JSON.parse expects a string, not an object, otherwise you'll get a SyntaxError as it will try to parse [object Object] from {}.toString().

Copy link
Contributor

@alessiodf alessiodf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the latest commit is only a wip so you might be aware of this already but I'm mentioning it here just in case as it will introduce some undesired effects with the api in its current incarnation.

@@ -94,7 +94,7 @@ class Httpie {
const { body, headers, statusCode } = await got[method](url, opts);

return {
body: JSON.parse(body || {}),
body: JSON.parse(body),
Copy link
Contributor

@alessiodf alessiodf Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: JSON.parse(body),
body: JSON.parse(body || "{}"),

Alternatively (might be more performant):

Suggested change
body: JSON.parse(body),
body: body ? JSON.parse(body) : {},

In its current state it will throw if body is empty or undefined since parsing will fail, which is always the case when the response is to a HEAD request. In real terms this means port pinging in the peer communicator will set all port values to -1 in the /api/peers endpoint.

@@ -21,7 +21,7 @@ export class HttpieError extends Error {
Object.defineProperty(this, "response", {
enumerable: false,
value: {
body: JSON.parse(error.response.body || {}),
body: JSON.parse(error.response.body),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
body: JSON.parse(error.response.body),
body: JSON.parse(error.response.body || "{}"),

Same.

@faustbrian faustbrian closed this Aug 10, 2020
@faustbrian faustbrian deleted the fast-json-parse branch August 10, 2020 09:26
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.

None yet

2 participants