Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

fix(index): escape invalid characters #43

Merged
merged 4 commits into from
Nov 17, 2017

Conversation

joeheyming
Copy link
Contributor

@joeheyming joeheyming commented Nov 14, 2017

Notable Changes

If your raw text has a string with Line separator or Paragraph separator,
they need to be handled outside of JSON.stringify:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_plain_JSON.stringify_for_use_as_JavaScript

The fix here is to replace those characters as directed in the MDN article above.

I also changed the formatting from tabs to 2 spaces :-p

Issues

If your raw text has a string with Line separator or Paragraph separator,
they need to be handled outside of JSON.stringify:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#Issue_with_plain_JSON.stringify_for_use_as_JavaScript

The fix here is to replace those characters as directed in the MDN article above.

I also changed the formatting from tabs to 2 spaces :-p
@jsf-clabot
Copy link

jsf-clabot commented Nov 14, 2017

CLA assistant check
All committers have signed the CLA.

@joeheyming
Copy link
Contributor Author

joeheyming commented Nov 14, 2017

This is related to Issue #42

@joeheyming
Copy link
Contributor Author

This CI failure has nothing to do with my changes

@michael-ciniawsky
Copy link
Member

raw-loader@beta should contain these changes, but we could/should backport them

@michael-ciniawsky michael-ciniawsky changed the title Handle special whitespace characters. fix(index): escape invalid characters Nov 15, 2017
@michael-ciniawsky michael-ciniawsky added this to the 0.5.2 milestone Nov 15, 2017
index.js Outdated
this.cacheable && this.cacheable();
this.value = content;

var stringified = JSON.stringify(content)
Copy link
Member

@michael-ciniawsky michael-ciniawsky Nov 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var stringified = ... => content = ... and maybe rename content to json please :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the content is the raw text of the file. It's not json

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, agreed src as argument and var json = JSON.stringify(src) is better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

index.js Outdated
this.value = content;

var stringified = JSON.stringify(content)
.replace(/\u2028/g, '\\u2028')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent chained methods with 2 spaces please

json = ...
  .method()

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.js Outdated
this.value = content;
return "module.exports = " + JSON.stringify(content);
module.exports = function(source) {
this.cacheable && this.cacheable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't support webpack 1.x. Cachable can be removed at this point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Turns out cacheable is only used in webpack 1.
So we are removing it.
.replace(/\u2028/g, '\\u2028')
.replace(/\u2029/g, '\\u2029');

return "module.exports = " + json;
Copy link

@stevemao stevemao Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just return "module.exports = " + '"' + source + '"'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the problem I had was that I was importing a file which had these special characters and it turns out that they break JSON. See the MDN article I posted above.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know. But why converting to json is better than wrapping the string with "?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, it's a matter of choice. I've been used to using json stringify for things like this where code gets evaluated. But yeah, you are right, it is the same.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax error with special whitespace characters
6 participants