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

Skip undefined properties for options.json = true; #52

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ziir
Copy link

@ziir ziir commented Jun 27, 2019

Hello, many thanks for this awesome package.

Similarily to JSON.stringify, jsesc(obj, { json:true }) should skip undefined properties within obj.

Side-note: Following discussions on Twitter, I was experimenting moving away from serialize-javascript to jsesc to serialize Redux SSR's initial state for GANDI apps such as https://shop.gandi.net when I encountered jsesc's behavior regarding undefined object properties, which differs from serialize-javascript's behavior.

Awaiting feedback before making final changes.

Fixes #17
Continuation of #18

Thank you for any response.

@mathiasbynens
Copy link
Owner

Please make the changes in /src/jsesc.js. /jsesc.js is generated from it when you run the build script.

@ziir ziir force-pushed the undefined-properties branch from c4cdf41 to 1613a09 Compare July 19, 2019 09:34
@ziir
Copy link
Author

ziir commented Jul 19, 2019

@mathiasbynens I have made the changes in the source, ran the build script and updated the README.
I'm not sure why the Travis build is failing though. Please advise.

Since recent changes, seems like Travis CI cannot run them within
25seconds
@ziir
Copy link
Author

ziir commented Jul 24, 2019

I just came across another case of different behavior between jsesc(..., { json: true }) (this PR's version) and JSON.stringify():

> require('jsesc')({ foo: () => {} }, { json: true })
'{"foo":null}'
> JSON.stringify({ foo: () => {} })
'{}'
> require('serialize-javascript')({ foo: () => {} }, { isJSON: true })
'{}'

I believe this falls in the scope of #17.

I will address it in the fork of jsesc I'm currently using for my project, and suggest a patch here.

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.

Document or remove differences between jsesc(data, { json: true }) and JSON.stringify(data)
2 participants