Closed abstractvector closed 6 years ago
Thanks @abstractvector I'll review this at the weekend. I see it's failing on some (ancient) versions of node! I'll likely just remove those and bump the major version number to denote a breaking change. Your other changes sound good - it's been a while since I touched this module and I probably did some things a bit weirdly all that time ago :-)
Apologies @andymantell! I didn't have those old versions installed to test locally. Per your comment above I've removed them from the .travis.yml
file and added an engines
directive to package.json
to show that only Node.js 4+ is supported. I've reissued the PR and the tests all now pass on Travis. The only thing I haven't done is major version bumped for you - all yours!
Looks good, I'll merge and do a release at lunchtime.
@andymantell I've removed the dependency on
phpjs
entirely - it was only being used in the tests and mostly just for itsphpjs.trim()
function with I replaced withString.trim()
. The only other one was on line 128, you were usingphpjs.htmlentities()
. That's a slightly less trivial one to replace - but I removed it from the test and the test still passed. Double check you're happy with that, and if not I'd suggest using the he library instead of phpjs.Whilst I was there, I also added
mocha
as a devDependency so you don't have to globally install it in your.travis.yml
file. I also updated the Travis config to include some more recent versions ofnode_js
.Finally, I removed the
mocha-jshint
devDependency since this was only being referenced in a commented out line of code in a test file (which I also removed, since it was empty except the comment). Not sure if you had been using this as a quick way tojshint
your code, but I'd advise either setting a proper build script to do that or installing it globally. I'm happy to put that back in if I've overstepped the mark by removing it, but this felt like an easy cleanup.