fix: escape newline/paragraph separators by bmeurer · Pull Request #36 · webpack-contrib/raw-loader

bmeurer · 2017-09-27T20:07:51Z

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.

jsf-clabot · 2017-09-27T20:07:53Z

All committers have signed the CLA.

mathiasbynens · 2017-09-27T20:20:58Z

-	return "module.exports = " + JSON.stringify(content);
+	content = JSON.stringify(content)
+		.replace(/\u2028/g, '\\u2028')
+		.replace(/\u2029/g, '\\u2029');


Alternatively, use jsesc if you want more control over the output (e.g. if you want completely ASCII-safe output).

michael-ciniawsky · 2017-09-27T20:28:35Z

+	content = JSON.stringify(content)
+		.replace(/\u2028/g, '\\u2028')
+		.replace(/\u2029/g, '\\u2029');
+	return `module.exports = ${content}`;


return 'module.exports = ' + content;

I was following the example of json-loader that @sokra pointed me to, where it's using template literals.

@bmeurer - Just skip the template for now. As soon as this gets pulled into the webpack-defaults branch it will be converted to a template anyway.

michael-ciniawsky

@bmeurer Thx

bmeurer · 2017-09-28T12:22:08Z

Anything missing to get this fixed?

alexander-akait · 2017-09-28T12:35:33Z

@bmeurer what about jsesc?

bmeurer · 2017-09-28T12:42:18Z

@evilebottnawi dunno. Maybe it makes sense to do this also for json-loader then.

alexander-akait · 2017-09-28T12:59:32Z

@bmeurer can you investigate this?

bmeurer · 2017-09-28T13:35:31Z

@evilebottnawi done

Similar to webpack-contrib/raw-loader#36 we can use jsesc here to deal with special characters in general.

michael-ciniawsky

Do we really need it ?

michael-ciniawsky · 2017-09-28T15:17:28Z

-	return "module.exports = " + JSON.stringify(content);
+	content = JSON.stringify(content);
+	content = jsesc(content);
+	return 'module.exports = ' + content;


If jsesc will be used we can go with a template literal here since the dependency requires node >= v4.0.0 anyways. node =< v4.0.0 is EOL, but this would be a SemVer Major for json-loader then.

If support for older environments is a concern, use jsesc v1.3.0: https://github.com/mathiasbynens/jsesc#support

Well it depends, since node =< v4.0.0 is EOL, it's more of a note and just needs consensus how to handle it (:label: patch/major && jsesc v1.3.0/latest). Let's wait for input of others on this matter :)

mathiasbynens · 2017-09-29T01:47:01Z

-		.replace(/\u2029/g, '\\u2029');
-	return `module.exports = ${content}`;
+	content = JSON.stringify(content);
+	content = jsesc(content);


Don’t use both! Doing so would cause over-escaping of \n etc.

In general, you can use jsesc(content, { json: true )) instead of JSON.stringify(content).

In this particular case, since we don’t really need JSON but rather a valid JS string literal, you could use jsesc(content, { wrap: true )) instead.

Would you mind reviewing json-loader #62 aswell please ?

michael-ciniawsky · 2017-09-29T02:41:36Z

Adding status: blocked for now as we need to find consensus on #36 (comment) and following before moving on with this

bmeurer · 2017-09-29T04:46:59Z

How about we merge the initial fix then first, since that already fixes the fundamental bug. Whether or not going for jsesc can be solved in a follow-up PR?

michael-ciniawsky · 2017-09-29T06:23:18Z

Yep, reverting is the best solution imho, we can do proper escaping with all the fuss evolved in a follow up if needed.

bmeurer · 2017-09-29T06:39:05Z

Ok, there you go. Just the simple bug fix.

michael-ciniawsky · 2017-10-01T02:19:12Z

@d3viant0ne I case #25 lands soon this would be good to go aswell together with #10

joshwiens · 2017-10-01T05:34:05Z

@michael-ciniawsky - Writing a few tests for this to go into #25 ( not that there is a lot to test ) but it would be nice to know if something broke completely.

@bmeurer - Can you rebase this so I can get this merged & pulled into #25

bmeurer · 2017-10-01T09:50:38Z

Ok, I'm a bit lost here. I changed it back to the initial version (w/o the string template). Feel free to either merge this into master or into whatever branch you want. But it'd be nice to finally get this trivial bugfix in.

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.

michael-ciniawsky · 2017-10-02T03:52:16Z

Yeah... sry this got somehow derailed quite a bit, we should get this in as soon as possible, #25 should be/is ready just needs a someone from the release team to publish it

bmeurer · 2017-10-02T03:57:45Z

Since #25 is unrelated/orthogonal to this, can you just merge the bugfix and release a fixed version?

I don't want to sound rude, but given that it'd be just a single click to get the actual bug fixed, I don't quite understand why this is open for more than 4 days, with discussion about orthogonal/unrelated issues?

joshwiens · 2017-10-02T05:04:31Z

@bmeurer - Next release is a semver: Major, this why all of this is going out as a single Major.

I'll split the difference with you and put out a beta build for 1.0.0

bmeurer · 2017-10-02T05:17:31Z

Thanks.

joshwiens · 2017-10-02T05:28:19Z

Alright, give me ~45 minutes to finish what I am working on & i'll get it out to npm.

bmeurer · 2017-10-10T09:54:33Z

Any chance to get this pushed to npm? It's really annoying to have to manually patch it all the time.

joshwiens · 2017-10-10T10:02:39Z

https://github.com/webpack-contrib/raw-loader/releases

mathiasbynens reviewed Sep 27, 2017

View reviewed changes

michael-ciniawsky added pr_action: Review semver: Patch severity: 2 (regression) type: Bug labels Sep 27, 2017

michael-ciniawsky reviewed Sep 27, 2017

View reviewed changes

michael-ciniawsky requested review from alexander-akait and joshwiens September 27, 2017 20:28

michael-ciniawsky approved these changes Sep 27, 2017

View reviewed changes

michael-ciniawsky added this to the 0.5.2 milestone Sep 27, 2017

bmeurer added a commit to bmeurer/json-loader that referenced this pull request Sep 28, 2017

Use jsesc to escape special characters.

aa2b684

Similar to webpack-contrib/raw-loader#36 we can use jsesc here to deal with special characters in general.

bmeurer mentioned this pull request Sep 28, 2017

refactor: use jsesc to escape special characters webpack-contrib/json-loader#62

Open

michael-ciniawsky reviewed Sep 28, 2017

View reviewed changes

mathiasbynens suggested changes Sep 29, 2017

View reviewed changes

michael-ciniawsky added the status: Blocked label Sep 29, 2017

bmeurer force-pushed the patch-1 branch from b92e64f to eb66965 Compare September 29, 2017 06:38

michael-ciniawsky added pr_action: Rework status: Pending and removed pr_action: Review status: Blocked labels Sep 30, 2017

michael-ciniawsky added pr_action: Merge ready semver: Major status: Approved and removed pr_action: Rework semver: Patch status: Pending labels Oct 1, 2017

joshwiens modified the milestones: 0.5.2, 1.0.0 Oct 1, 2017

fix: escape newline/paragraph separators

8f9816e

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.

bmeurer force-pushed the patch-1 branch from 4d69e2a to 8f9816e Compare October 1, 2017 09:55

michael-ciniawsky mentioned this pull request Oct 2, 2017

refactor: apply webpack-defaults #25

Closed

joshwiens merged commit c972c92 into webpack-contrib:master Oct 2, 2017

michael-ciniawsky removed pr_action: Merge ready severity: 2 (regression) labels Oct 2, 2017

Uh oh!

Conversation

bmeurer commented Sep 27, 2017

Uh oh!

jsf-clabot commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mathiasbynens Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

bmeurer Sep 27, 2017

Choose a reason for hiding this comment

Uh oh!

joshwiens Oct 1, 2017

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

bmeurer commented Sep 28, 2017

Uh oh!

alexander-akait commented Sep 28, 2017

Uh oh!

bmeurer commented Sep 28, 2017

Uh oh!

alexander-akait commented Sep 28, 2017

Uh oh!

bmeurer commented Sep 28, 2017

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Sep 28, 2017

Choose a reason for hiding this comment

Uh oh!

mathiasbynens Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

mathiasbynens Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Sep 29, 2017

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky commented Sep 29, 2017

Uh oh!

bmeurer commented Sep 29, 2017

Uh oh!

michael-ciniawsky commented Sep 29, 2017

Uh oh!

bmeurer commented Sep 29, 2017

Uh oh!

michael-ciniawsky commented Oct 1, 2017

Uh oh!

joshwiens commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmeurer commented Oct 1, 2017

Uh oh!

michael-ciniawsky commented Oct 2, 2017

Uh oh!

bmeurer commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshwiens commented Oct 2, 2017

Uh oh!

bmeurer commented Oct 2, 2017

Uh oh!

joshwiens commented Oct 2, 2017

Uh oh!

bmeurer commented Oct 10, 2017

Uh oh!

jsf-clabot commented Sep 27, 2017 •

edited

Loading

mathiasbynens Sep 29, 2017 •

edited

Loading

mathiasbynens Sep 29, 2017 •

edited

Loading

joshwiens commented Oct 1, 2017 •

edited

Loading

bmeurer commented Oct 2, 2017 •

edited

Loading