Implement `allowUnsafeSymbols` option (which defaults to `false`) for `he.encode` #23

jugglinmike · 2014-08-07T13:02:23Z

I'd love to use this library in my polyfill for the iframe srcdoc attribute, but I think I need an additional feature. The functionality I'm proposing here would help me to resolve an issue that was recently filed against that project.

Commit message:

Although he.escape allows for escaping "unsafe" markup characters,
there is currently no way to escape the inverse set--non-ASCII
characters only. This is useful in contexts where markup is allowed, but
non-ASCII characters might be otherwise mangled (as in the iframe
srcdoc attribute, for example).

Introduce a new option to he.encode which disables the escaping of
unsafe markup characters. Preserve API compatability by disabling this
behavior by default.

mathiasbynens · 2014-08-07T13:30:42Z

Your patch looks good so far. Some remarks:

I’d like to rename markupChars to something that more closely matches the other option names, though – escapeUnsafeSymbols for example. What do you think? Any suggestions?
More tests are needed that show how this new option combines with the other existing options (such as escapeEverything).
The CLI needs to support this new option too.

By the way, this would solve #16.

Note that this new option should not be used in cases where the markup can contain custom elements, since those can contain all kinds of symbols and escaping e.g. <λ-calculus> as <λ-calculus> would break stuff. The same goes for non-HTML contexts, such as the contents of a <script> or <style> element (you don’t want to HTML-escape anything in there). Perhaps this should be added to the docs as a warning?

jugglinmike · 2014-08-16T15:06:08Z

@mathiasbynens I've renamed the option, but I have a few questions about your other requests:

This is an option for the encode method and the library already supports an option named encodeEverything, so what about naming this option encodeUnsafeSymbols instead of escapeUnsafeSymbols?
How should the library behave when ~~both encodeEverything and this new option are set to true~~ encodeEverything is set to true and this new option is set to false? There is a little ambiguity here because both options control the same behavior, just on different character sets. I could see it encoding everything except unsafe characters, but I could also see "everything" taking precedence, resulting in everything being encoded (in other words, ignoring escapeUnsafeSymbols). Because of the ambiguity, I think either decision could be interpreted as arbitrary, so I'm inclined to throw an error if both are set. Do you think that's the right approach?
It doesn't look like the project explicitly tests the CLI--could you confirm?

mathiasbynens · 2014-08-16T21:16:35Z

@jugglinmike

Good call! +1 to encodeUnsafeSymbols rather than escape….
IMHO encodeEverything should take precedence, we should update the documentation to reflect this, and not throw an error (i.e. silently do what the documentation says it does). No one is going to use these options without reading the documentation anyway. Perhaps the problem could be avoided by renaming encodeEverything into something like encodeSafeAsciiSymbols or encodeAsciiSymbols?
At the moment the CLI does not have tests, indeed.

Thanks for your work on this so far!

jugglinmike · 2014-08-17T01:35:49Z

@mathiasbynens Thanks for the clarification. I've held off on renaming the existing method because I'm not sure it resolves the ambiguity, and the suggestion seemed more like an afterthought. Of course, I'm happy to change it if you would like me to!

The encodeUnsafeSymbols option is a little unique in that it defaults to true, and reflecting this in the CLI presented a little bit of a problem. I decided that consistency between the flags within the CLI (via --allow-unsafe) is more important than consistency across the CLI and JavaScript API (via, for instance, --unsafe=false). I'm not sure if that matches how you would design it, so I wanted to call it to your attention.

By the way, I expect you'll want to squash all this down when we get to a final draft. I'd be happy to do that too, just let me know.

mathiasbynens · 2014-08-17T08:44:05Z

README.md

@@ -106,7 +106,7 @@ he.encode('foo © bar ≠ baz 𝌆 qux', {

 #### `encodeEverything`

-The default value for the `encodeEverything` option is `false`. This means that `encode()` will not use any character references for printable ASCII symbols that don’t need escaping. Set it to `true` to encode every symbol in the input string.
+The default value for the `encodeEverything` option is `false`. This means that `encode()` will not use any character references for printable ASCII symbols that don’t need escaping. Set it to `true` to encode every symbol in the input string. When set to `true`, this option will take precedence over `encodeUnsageSymbols` (setting the latter to `false` will have no effect).


Typo: encodeUnsafeSymbols

mathiasbynens · 2014-08-17T08:49:05Z

@jugglinmike Your arguments convinced me that allowUnsafeSymbols would actually be an even better name (because then the default value is false, like all other options). 👍 (I don’t like the double negative in allowUnsafeSymbols: false but hey.)

jugglinmike · 2014-08-17T14:08:40Z

Thanks for the feedback, @mathiasbynens — I think I've addressed it all.

mathiasbynens · 2014-08-18T08:07:25Z

README.md

+he.encode('foo © and & ampersand');
+// → 'foo &#xA9; and & ampersand'
+```
+


The docs for he.encode() should be updated too (maybe the first paragraph can just get a link to this section). I can do this after merging your PR, no worries.

mathiasbynens · 2014-08-18T08:11:11Z

This looks great! Could you squash your commits please?

If you don’t want to fix those last few nitpicks I just left as comments, no worries, I’m happy to take care of them after merging your patch. Just let me know.

jugglinmike · 2014-08-19T11:29:50Z

No problem at all, Mathias. I am a bit busy this week, so I don't expect to address these final issues until Saturday. If you'd like to see this merged before then, though, be my guest :)

Although `he.escape` allows for escaping "unsafe" markup characters, there is currently no way to escape the inverse set--non-ASCII characters only. This missing functionality is useful in contexts where markup is allowed, but non-ASCII characters might be otherwise mangled (as in the iframe `srcdoc` attribute, for example). Introduce a new option to `he.encode` which disables the escaping of unsafe markup characters. Preserve API compatability by disabling this behavior by default.

coveralls · 2014-08-24T04:35:12Z

Coverage increased (+0.07%) when pulling 37f0e29 on jugglinmike:markupchars-option into 78bc2bb on mathiasbynens:master.

jugglinmike · 2014-08-24T04:36:20Z

@mathiasbynens I've fixed the typo, squashed the commits, and updated the commit message. I held off on the README.md change you requested because I'm not sure how to best address it. It will probably save us a few more rounds of review if I leave that to you :)

Although `he.escape` allows for escaping ‘unsafe’ markup characters, there is currently no way to escape the inverse set – non-ASCII characters only. This missing functionality is useful in contexts where markup is allowed, but non-ASCII characters might be otherwise mangled (as in the `iframe` `srcdoc` attribute, for example). Introduce a new option to `he.encode` which disables the escaping of unsafe markup characters. Preserve API compatability by disabling this behavior by default. Closes #16 and #23.

mathiasbynens · 2014-08-24T07:00:25Z

Thanks, I’ve tweaked your patch and merged it. Will release a new version now.

$ npm install he@0.5.0 --save-dev

Although `he.escape` allows for escaping ‘unsafe’ markup characters, there is currently no way to escape the inverse set – non-ASCII characters only. This missing functionality is useful in contexts where markup is allowed, but non-ASCII characters might be otherwise mangled (as in the `iframe` `srcdoc` attribute, for example). Introduce a new option to `he.encode` which disables the escaping of unsafe markup characters. Preserve API compatability by disabling this behavior by default. Closes #16 and #23.

jugglinmike · 2014-08-24T15:49:35Z

Awesome--thanks!

jugglinmike mentioned this pull request Aug 7, 2014

IE9 encoding issue jugglinmike/srcdoc-polyfill#5

Open

mathiasbynens reviewed Aug 17, 2014
View reviewed changes

mathiasbynens reviewed Aug 18, 2014
View reviewed changes

jugglinmike force-pushed the markupchars-option branch from c6d1e9b to 37f0e29 Compare August 24, 2014 04:33

mathiasbynens changed the title ~~Implement markupChars option for he.encode~~ Implement allowUnsafeSymbols option (which defaults to false) for he.encode Aug 24, 2014

mathiasbynens closed this Aug 24, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `allowUnsafeSymbols` option (which defaults to `false`) for `he.encode` #23

Implement `allowUnsafeSymbols` option (which defaults to `false`) for `he.encode` #23

jugglinmike commented Aug 7, 2014

mathiasbynens commented Aug 7, 2014

jugglinmike commented Aug 16, 2014

mathiasbynens commented Aug 16, 2014

jugglinmike commented Aug 17, 2014

mathiasbynens Aug 17, 2014

mathiasbynens commented Aug 17, 2014

jugglinmike commented Aug 17, 2014

mathiasbynens Aug 18, 2014

mathiasbynens commented Aug 18, 2014

jugglinmike commented Aug 19, 2014

coveralls commented Aug 24, 2014

jugglinmike commented Aug 24, 2014

mathiasbynens commented Aug 24, 2014

jugglinmike commented Aug 24, 2014

Implement allowUnsafeSymbols option (which defaults to false) for he.encode #23

Implement allowUnsafeSymbols option (which defaults to false) for he.encode #23

Conversation

jugglinmike commented Aug 7, 2014

mathiasbynens commented Aug 7, 2014

jugglinmike commented Aug 16, 2014

mathiasbynens commented Aug 16, 2014

jugglinmike commented Aug 17, 2014

mathiasbynens Aug 17, 2014

Choose a reason for hiding this comment

mathiasbynens commented Aug 17, 2014

jugglinmike commented Aug 17, 2014

mathiasbynens Aug 18, 2014

Choose a reason for hiding this comment

mathiasbynens commented Aug 18, 2014

jugglinmike commented Aug 19, 2014

coveralls commented Aug 24, 2014

jugglinmike commented Aug 24, 2014

mathiasbynens commented Aug 24, 2014

jugglinmike commented Aug 24, 2014

Implement `allowUnsafeSymbols` option (which defaults to `false`) for `he.encode` #23

Implement `allowUnsafeSymbols` option (which defaults to `false`) for `he.encode` #23