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

Add 'json escape' option to unicode escape <, >, and & #3269

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@g-k
Contributor

g-k commented Apr 5, 2017

refs: #3268

@dougwilson

So I am reviewing this with the assumption that #3268 will result in the desire to add this feature, but you can wait on that discussion to fix the issue if you like :)

I would expect that res.jsonp would respect this option, if you can fix that (i.e. I would expect anywhere in Express that respects json replacer, json spaces, etc. to also honor this new json escape).

g-k added some commits Apr 12, 2017

use single replace call like DavidTPate recommended for hoek
could use the char code version too
@g-k

This comment has been minimized.

Show comment
Hide comment
@g-k

g-k Apr 12, 2017

Contributor

Added the setting to jsonp and git grep doesn't show the json options used elsewhere.

Updated to use a single String.replace call that a similar change to hapijs/hoek showed as ~3x faster on the twitter.json test data.

Contributor

g-k commented Apr 12, 2017

Added the setting to jsonp and git grep doesn't show the json options used elsewhere.

Updated to use a single String.replace call that a similar change to hapijs/hoek showed as ~3x faster on the twitter.json test data.

@dougwilson dougwilson modified the milestones: 5.0, 4.16 Jul 19, 2017

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Jul 19, 2017

Member

Unless there are any objections, I'm suggesting that this land in the 4.16.0 release.

Member

dougwilson commented Jul 19, 2017

Unless there are any objections, I'm suggesting that this land in the 4.16.0 release.

@dougwilson

Final comments added. Ideally all changed code adheres to the StandardJS style, but I can alter your changes on landing if you don't want to change that up.

Show outdated Hide outdated lib/response.js
Show outdated Hide outdated lib/response.js
Show outdated Hide outdated test/res.json.js
Show outdated Hide outdated test/res.jsonp.js

g-k added some commits Jul 20, 2017

@g-k

This comment has been minimized.

Show comment
Hide comment
@g-k

g-k Jul 20, 2017

Contributor

Thanks @dougwilson! I think I got all of the requested changes, but please modify the PR as necessary to get it in shape too.

Contributor

g-k commented Jul 20, 2017

Thanks @dougwilson! I think I got all of the requested changes, but please modify the PR as necessary to get it in shape too.

@dougwilson dougwilson referenced this pull request Sep 25, 2017

Merged

Release 4.16 #3423

20 of 20 tasks complete

@dougwilson dougwilson self-assigned this Sep 28, 2017

@dougwilson dougwilson added 4.x enhancement and removed ideas labels Sep 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment