Rewrite using promises and less dependencies

[SamuelTilly]

This project introduces a lot of dependencies. This is an example of css.js without much of the dependencies. If approved i would like to make the same changes to html.js and remove most of the dependencies all together.

Promises is part of node 4.2.0 so it should be safe to use instead of async.

• Remove async dependency
• Remove lodash.constant dependency
• Remove xtend dependency
• Added test for inline-ignore in CSS
• Only search content once before replacing and rebasing
• Returning a promise, making callback optional

[jrit]

Thanks for the PR. A few thoughts here:

• Removing xtend as a dependency is requiring copying basically the same code into the project, which IMO isn't an improvement, the total library size is the same, but it increases the maintenance cost
• Without bike shedding promises in Node too much, because people certainly do, please don't have it return the promise if the callback is defined, that dual interfacing creates too much opportunity for users to make mistakes and handle callbacks twice.
• The internal use of Promise and removing async looks good to me, that is a big dependency. I was playing recently with requiring just require( "async" ).parallel but that is still a big npm install, so your way is better IMO.
• Thanks for getting rid of lodash.constant 👍

Feel free to push more commits into this PR and let me know how you feel about this feedback. Thanks again.

[SamuelTilly]

Thanks for the feedback!

The xtend dependency comes with it's own package,json, tests and two versions of the same code, immutable and mutable. With a total size of 7kB. It's not that much but for projects like mine this kinds of dependencies stack up. I'll re-add it if you like but i see no real reason to have it, please let me know.

How do you feel about clean-css? It's nice to have the css cleaned but i would argue that it's not within the scope of this project. perhaps have a similar hook like the scriptTransform and let people choose their own css cleaning library?

I have made the same changes to the HTML part with the biggest change is utilizing htmlparser2 for all HTML parsing, this should be more reliable then using regex but changes the output a bit.

There is a couple of things i would like to do before I'll consider this code ready.

1. Use fs.readFile instead of fs.readFileSync.
2. Precalculate sources for HTML (not requesting the same source twice).
3. CSS should be able to lookup inlineAttribute in the initial search.

[jrit]

I agree about clean-css. That has already been removed in master though in favor of using linkTransform, unless I missed it somewhere?

In reference to xtend, I was really talking about the memory footprint vs the amount on disk. With require("xtend") it only needs to load the immutable, which is essentially the same code that was copied in. So, still, I'd rather use an npm dependency than have the same code copied into a local project file.

I'm not super-familiar with how htmlparser2 works internally, but my concern with moving away from regex is causing changes to the code formatting such as you see. Unfortunately, based on what I know about how people use this library, I don't think those changes required to the tests are acceptable.

[SamuelTilly]

I'll re-add xtend for you.

I was referring to settings.cssmin used in css.js.

Alright, i could make some changes to the output from htmlparser2 so that the output is more similar to what is expected. Is there something in particular with the tests that needs to be reverted?

• I think that adding type for script and style might be a bit too much but i can't see that it would hurt.
• svg now actually inserts the element with the specified id.
• img will replace invalid /> with >.
• empty attributes will be correctly written with =""

For the CSS output it now only replaces the content of a url() and not the whole string.

I fixed issue #7 and #21 as an added bonus

[jrit]

The cssmin setting should be removed, you are right, since it shouldn't be used anymore.

For the change to htmlparser2, I'm still very worried about unacceptable breakage from that. juice is the biggest library that uses web-resource-inliner and changes in this library should not cause problematic breakage in the juice test suite, but I'm pretty sure they will in some places. Area you able to run a version of juice against your updates and see?

Do you mind reverting the htmlparser2 changes so we can merge all that other good stuff and continue the conversation? I didn't think you'd be adding that as a dependency since you earlier stated you were trying to remove dependencies.

[SamuelTilly]

You are probably right, this would require a bump in the major version in both juice and web-resource-inliner to make sure there will is no breakages.

htmlparser2 is already used in the master version of web-resource-inliner for parsing SVG, so I'm not adding a new dependency, simply using the once we have in a better way.

I could probably change the replacing part back to the original regex way and make the tests compatible with the current once, but i think that ultimately regex is never a good solution for parsing HTML reliably.