Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements an option to prevent the automatic encoding #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

halbgut
Copy link

@halbgut halbgut commented Dec 26, 2015

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

@ErisDS
Copy link
Collaborator

ErisDS commented Jan 25, 2016

This change looks good 👍

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.

@halbgut
Copy link
Author

halbgut commented Jan 25, 2016

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>' }  } ]

@ErisDS
Copy link
Collaborator

ErisDS commented Jan 25, 2016

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): dylang/node-rss#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.

@halbgut
Copy link
Author

halbgut commented Jan 25, 2016

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
Copy link
Author

halbgut commented Mar 20, 2016

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?

@jamsyoung
Copy link

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

@ErisDS
Copy link
Collaborator

ErisDS commented Apr 18, 2016

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!

@halbgut
Copy link
Author

halbgut commented Apr 19, 2016

Great! Thanks a lot. :)

I'll probably get to it today.

@halbgut
Copy link
Author

halbgut commented Apr 19, 2016

Thanks a lot @ErisDS for the instructions :)

@halbgut
Copy link
Author

halbgut commented Jun 28, 2016

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

@bnetter
Copy link

bnetter commented Sep 8, 2016

We need this too!

@soujiro32167
Copy link

Bump! Looking forward to this

@mhaneef50673
Copy link

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

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

Successfully merging this pull request may close these issues.

6 participants