search

remy / inliner

Node utility to inline images, CSS and JavaScript for a web page - useful for mobile sites
MIT License
1.1k stars 165 forks source link

Treat styles and scripts as text #53

Closed hsablonniere closed 9 years ago

hsablonniere commented 9 years ago

Hi Remy,

I tried inliner on a page with a huge external script and I encountered a strange problem. The JavaScript output was not the same. I managed to find what was wrong and reduced the problem to scripts containing characters like < or >. It seems that when you use cheerio's .html() setter here, it's treated as if it could contain some HTML tags.

I replaced it with a .text() setter and I updated the existing tests. I thought creating new tests for that was not necessary but I'll let you judge ;-)

I didn't encounter it but the same problem can happen with CSS and stuffs like content: "<" so I also fixed it and updated the tests.

Cheers...

hsablonniere commented 9 years ago

It seems to be a bit more complex :-(

My page also has highlight.js as an external script and this lib contains </script> in JavaScript strings. As you can imagine, it brakes the inlining...

So the fix here prevents inliner to replace some characters by their escaped HTML equivalent but we still face the problems of ETAGO.

@mathiasbynens says in his article, we should just replace </ with <\/.

WDYT?

hsablonniere commented 9 years ago

I checked and the replace (</script with <\/script) fixes my issue!!

The only problem is that the current tests cannot "see" this problem. The HTML would have to be parsed/executed in a browser to see the errors in the logs...

remy commented 9 years ago

Looks good :+1:

hsablonniere commented 9 years ago

@remy Thanks for the merge. What do you think about the external scripts that contain </script>?

hsablonniere commented 9 years ago

Sorry about the ETAGO confusion. It's a different issue and the bug has already been filed by Mathias Bynens himself in #11.

I proposed a fix in #54.