Closed jugglinmike closed 10 years ago
Your patch looks good so far. Some remarks:
markupChars
to something that more closely matches the other option names, though – escapeUnsafeSymbols
for example. What do you think? Any suggestions?escapeEverything
).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?
@mathiasbynens I've renamed the option, but I have a few questions about your other requests:
encode
method and the library already supports an option named encodeEverything
, so what about naming this option encodeUnsafeSymbols
instead of escapeUnsafeSymbols
? 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?@jugglinmike
encodeUnsafeSymbols
rather than escape…
.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
?Thanks for your work on this so far!
@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.
@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). :+1: (I don’t like the double negative in allowUnsafeSymbols: false
but hey.)
Thanks for the feedback, @mathiasbynens — I think I've addressed it all.
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.
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 :)
@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 :)
Thanks, I’ve tweaked your patch and merged it. Will release a new version now.
$ npm install he@0.5.0 --save-dev
Awesome--thanks!
Hey @mathiasbynens!
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: