search

jrit / web-resource-inliner

Inlines img, script and link tags into the same file
MIT License
66 stars 29 forks source link

(Don't merge) Inline css issue #40

Closed tommedema closed 2 years ago

tommedema commented 6 years ago

What better way to submit an issue than to submit a failing test case?

As illustrated, inline css is not being parsed at the moment, causing background images there to not be embedded.

How would you prefer to proceed here? Any suggestions on how this should be tackled?

Off topic:

P.S. I have numerous fixes lined up in other branches, but am not yet sending them because I am creating an end to end test suite with about a hundred sites to battle test this library. Once I feel like it's stable enough I will send a PR. Among others it:

  1. mocks http for all tests rather than using the file system: this is necessary because http resolves differently from local file systems. E.g. if you have a domain example.com, and a path like /image, it should resolve to example.com/image, rather than resolving to the root of your file system. Only url.resolve can work with this, requiring you to work with an actual url rather than a file path.

  2. I have moved all of your tests that require a network connection (ie make remote calls) to a separate remote suite; unit tests really shouldn't require an internet connection

  3. properly encodes uris prior to making requests; this resolved many issues related to 404s

  4. the biggest improvements are in the way resolving is done: you are resolving relative to the baseUrl (relativeTo), but this does not make sense when an external stylesheet is embedded (on another domain). I've fixed this as well.

jrit commented 6 years ago

This makes sense. I'll tell you the history of this behavior is that I wrote this module to be used with juice, and juice doesn't inline from style attributes, it only inlines from link or style tags. You are absolutely correct that this change makes sense because this module is its own thing. For the sake of not breaking upstream users, when you implement this (I hope you will!!), please include an option to not inline from within style attributes. I think it should default to the behavior that fixes this test here.

I'm happy to look at any of your feature branches or any PR whenever you are ready. Thanks for your hard work on this.