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

leading whitespace #21

Closed
freasy opened this issue Nov 24, 2022 · 3 comments · Fixed by #23
Closed

leading whitespace #21

freasy opened this issue Nov 24, 2022 · 3 comments · Fixed by #23
Assignees

Comments

@freasy
Copy link

freasy commented Nov 24, 2022

TL;DR: After updating "a" laravel package our app broke because destr evaluates all responses as string.

We are using lates laravel as backend with a fairly new plain vue3 frontend.

We are using ofetch (former ohmyfetch) for fetching api responses.
After a update laravel resources responses now have a leading whitespace, thats the reason destr evaluates the response as a string.

How to reproduce:

destr('{}') // type object
destr(' {}') // type text

Is is possible to trim the data before try parsing it?

@freasy freasy changed the title Standard laravel resource responses and prepending whitespace leading whitespace Nov 24, 2022
@NozomuIkuta
Copy link
Member

RFC8259 says that JSON text is a "value" optionally surrounded by white spaces (Section 2: JSON Grammer).

So, ' {}' should be parsed as a valid JSON, and actually JSON.parse() returns an object.

If I'm correct, the root cause is the regular expression below.

const JsonSigRx = /^["[{]|^-?\d[\d.]{0,14}$/;

It should accept white spaces.

I could fix the bug quickly, but it seems that we need unit tests first, otherwise we can't verify the bug fix.

@pi0 pi0 closed this as completed in #23 Dec 2, 2022
@NozomuIkuta NozomuIkuta reopened this Dec 2, 2022
@NozomuIkuta
Copy link
Member

NozomuIkuta commented Dec 2, 2022

Sorry, this issue was closed against my will, because I happened to use GitHub's magic comment in the PR (i.e., "fix #number").🙌

Let me reopen this issue and I will submit a PR to really fix this issue.

@NozomuIkuta NozomuIkuta self-assigned this Dec 3, 2022
@pi0 pi0 closed this as completed in 639a5df Dec 5, 2022
@pi0
Copy link
Member

pi0 commented Dec 5, 2022

Fixed in 639a5df (sorry @NozomuIkuta quickly added fix for next release)

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 a pull request may close this issue.

3 participants