node-xml icon indicating copy to clipboard operation
node-xml copied to clipboard

Implements an option to prevent the automatic encoding

Open halbgut opened this issue 9 years ago • 13 comments

I'm implementing an Atom feed generator and I'd like to be able to disable the automatic string encoding.

halbgut avatar Dec 26 '15 19:12 halbgut

This change looks good :+1:

Would you mind dropping a bit more of an explanation, including references, for the use-case here? This will help with documentation and other people trying to do the same thing.

ErisDS avatar Jan 25 '16 12:01 ErisDS

Sure! This PR adds a global escape option. When set to true, special characters in strings passed to xml won't be escaped to HTML-entities. This could be used for many things. One example is atom-feed entries. They can have a type="html" attribute. In most use cases you would get the HTML as a string and render the feed using node-xml. That's when you'd want to disable escaping. Making it a global option is simply the easiest way to implement that.

Here's a usage example I used in the tests:

xml(
  [ { x: '<a>test</a>' } ],
  { escape: false }
)

This would return

<x><a>test</a></x>

We'd probably also want a local way to do this. Similar to how _cdata works:

[ { x: { _raw: '<a>test</a>' }  } ]

halbgut avatar Jan 25 '16 14:01 halbgut

I'm interested to understand fully the reason for doing this in atom - you say "[feed entries] can have a type="html" attribute.", I'm trying to understand the difference between "can" and "should", is there a reference in the atom docs or to something about best practice that you can link here?

There is a very related issue on the node-rss repo (which directly depends on node-xml): https://github.com/dylang/node-rss/issues/27 - node-rss is supposed to output atom-compatible feeds, and the discussion there suggests that escaping should happen only if there are chars which need escaping, E.g. an 'auto' mode rather than on/off.

Just trying to get an idea of what the best path forward is for both node-xml and node-rss in respect to this issue.

ErisDS avatar Jan 25 '16 14:01 ErisDS

Sorry, I may have been a bit wage in my explanation. Text elements can have an attribute type="text|html|xhtml". So the feed reader would interpret the content as HTML.

If type="html", then this element contains entity escaped html.

Here's the relevant part on their Website

The example on that page isn't very clear. But as I interpret it, you could do

<content type="html">
  <em>Yol&ouml;</em>
</content>

That <em>Yol&ouml;</em> would in most cases come from some CMS or something. So you could simply use it as is inside the <content> of an article inside the Atom-Feed.

As far as I understand the problem in dylang/node-rss#27 is only remotely related. The auto detection he's talking about isn't something that I would include in node-xml itself (except maybe as an option), since it would give the programmer less control.

halbgut avatar Jan 25 '16 15:01 halbgut

I wanted to check up on the state of this issue, since the Atom feed on my project is invalid ATM.

Will this get merged?

halbgut avatar Mar 20 '16 15:03 halbgut

I also have a project that would benefit from this additional functionality.

jamsyoung avatar Apr 15 '16 14:04 jamsyoung

I'm sorry, for some reason all the notifications from this repo started going into my spam folder so I had no idea there'd been any followup!

This PR currently has a merge conflict. If we could get that fixed up, then I'll look at pushing this out.

This PR is based from a master branch, making it slightly tricker than normal to fixup, but these steps should work assuming your repo is called origin and this one upstream in your remotes config:

  • git fetch upstream (fetch from this repo)
  • git pull -r upstream master (rebase in latest changes from this repo)
  • resolve any conflicts (I think it's just a newline at the end of xml.js)
  • git push origin master -f (push the update to this PR)

Hopefully this gets us moving again!

ErisDS avatar Apr 18 '16 11:04 ErisDS

Great! Thanks a lot. :)

I'll probably get to it today.

halbgut avatar Apr 19 '16 06:04 halbgut

Thanks a lot @ErisDS for the instructions :)

halbgut avatar Apr 19 '16 16:04 halbgut

Is there still something that has to be done, in order for this PR to be merged?

halbgut avatar Jun 28 '16 19:06 halbgut

We need this too!

bnetter avatar Sep 08 '16 08:09 bnetter

Bump! Looking forward to this

soujiro32167 avatar May 27 '18 21:05 soujiro32167

Will this PR ever be merged? @halbgut @dylang ..We need this!

mhaneef50673 avatar Aug 31 '21 11:08 mhaneef50673