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

fix escapeJavaScript for quotes: double escape the double quote #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jlongman
Copy link

@jlongman jlongman commented Nov 8, 2021

ref: #8

double quote the string used in util.escapeJsonString.

Note, I notice the existing test - https://github.com/jlongman/api-gateway-mapping-template/blob/master/test/util.js#L5 - this would presumably break this test? I may add the test in the original issue to the PR.

Thinking about this, maybe this suggests a difference in runtime environments? Will add notes to the issue.

@jlongman
Copy link
Author

jlongman commented Nov 8, 2021

ack realized I forgot the conventional commit style, will try to revise but I'm in the browser atm.

jlongman and others added 3 commits November 9, 2021 09:05
ref: ToQoz#8

double quote the string used in `util.escapeJsonString`.

Note, I notice the existing test - https://github.com/jlongman/api-gateway-mapping-template/blob/master/test/util.js#L5 - this would presumably break this test? I may add the test in the original issue to the PR.

Thinking about this, maybe this suggests a difference in runtime environments?  Will add notes to the issue.
@jlongman jlongman force-pushed the 8-double-escape-quotes-in-escapeJavaScript branch from c78c85e to 6304feb Compare November 9, 2021 14:26
@jlongman
Copy link
Author

jlongman commented Nov 9, 2021

branch is updated with an additional test that embeds the bo"dy value into a JSON document and then parses it. This fails until the change is added. Then the original test is corrected to reflect the new reality.

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

1 participant