Escape Newline/Paragraph separators #18
Conversation
|
Demo of a failing import: Click on "Log" at the top left to see the error generated. |
|
@ajhyndman you should rebase and sign the CLA |
|
Christ. @ajhyndman: I'm happy to do it if you give me push access to your fork |
index.js
Outdated
| return "module.exports = " + | ||
| JSON.stringify(value, undefined, "\t") | ||
| .replace(/\u2028/g, '\\u2028') | ||
| .replace(/\u2029/g, '\\u2029') + |
There was a problem hiding this comment.
replace with string replaces only once. You need the regexp with g flag.
There was a problem hiding this comment.
oh my goodness how did I not know that, that's really surprising
|
@laughinghan Sure, thanks! |
|
@ajhyndman: you gotta give me push access to your fork though |
I think that the Webpack JSON loader seems like the right place to handle this issue. For whatever reason, the JSON and Javascript specifications disagree on whether or not strings can contain the unicode Newline or Paragraph characters. In the case that a JSON object containing one of these characters is being printed to a script tag, we should escape them. See also, this discussion: expressjs/express#1132 (comment)
laughinghan
force-pushed
the
json-is-not-a-js-subset
branch
from
be7abf1
to
76bd270
Compare
|
@graingert @sokra Rebased! |
|
@ajhyndman Can you please close and reopen the PR to trigger the CLA Bot again ? 😛 |
|
@ajhyndman: I don't think you need to do that, just go to https://cla.js.foundation/webpack-contrib/json-loader?pullRequest=18 and sign it and come back and I'd expect it to update (I've just done so, but I'm committer, you're author) |
|
@bebraw Sure, but there are no existing tests here. it's unreasonable to expect a contributor to write the whole testing infrastructure. |
|
@graingert Ah, yeah. Fair point. Two options:
|
|
@bebraw I'm working on various webpack-defaults upgrades for the 'smaller' loaders, but it will take a few days, it's not best pratice by any means but @graingert has a point here, since there isn't any test setup provided for contribs, we can always revert this change if the issue tracker blows up but thats my personal opinion (I don't think so) 😛 |
|
@michael-ciniawsky Yeah, it's a simple change so merge now. Easy enough to add a good test later. |
|
@ajhyndman Please close and reopen the PR to trigger the CLA Bot again , otherwise I can't merge this for legislative reasons 😛 |
|
|
|
@ajhyndman ping 😛 |
|
Sorry! Signed, and fixed up merge conflicts. |
Similar to webpack-contrib/json-loader#18 the `raw-loader` also needs to properly escape the special unicode characters, as otherwise `webpack` get's highly confused when it sees one of them.
Similar to webpack-contrib/json-loader#18 the `raw-loader` also needs to properly escape the special unicode characters, as otherwise `webpack` get's highly confused when it sees one of them.
Similar to webpack-contrib/json-loader#18 the `raw-loader` also needs to properly escape the special unicode characters, as otherwise `webpack` get's highly confused when it sees one of them.
Similar to webpack-contrib/json-loader#18 the `raw-loader` also needs to properly escape the special unicode characters, as otherwise `webpack` get's highly confused when it sees one of them.
I think that the Webpack JSON loader seems like the right place to handle
this issue.
For whatever reason, the JSON and Javascript specifications disagree on
whether or not strings can contain the unicode Newline or Paragraph
characters.
In the case that a JSON object containing one or more of these characters is
being printed to a script tag, we should escape them.
See also this discussion:
expressjs/express#1132 (comment)