search

mde / ejs

Embedded JavaScript templates -- http://ejs.co
Apache License 2.0
7.71k stars 846 forks source link

Fix #510. Remove jake dev dependency and embed arg parser. #645

Open arthanzel opened 2 years ago

arthanzel commented 2 years ago

EJS CLI introduced jake as a prod dependency. The only reason for this was to use jake's argument parsing code. This PR embeds the code into utils.js instead, makes jake into a dev dependency, and returns EJS to zero prod dependencies.

See https://github.com/jakejs/jake/blob/master/lib/parseargs.js

The argument parsing code is simple and modular. I don't think that creating yet another package for the EJS CLI is the way to go here to avoid an easily refactored dependency.

Though the argument parsing code is the same, the CLI is still missing tests that cover all the options. I do plan on adding these tests later.

mde commented 2 years ago

Thanks for making progress on this, but I need tests before I can even consider merging.

dev-trilobyte commented 2 years ago

this will help on #659 too

hppycoder commented 2 years ago

@mde - I pulled the branch from @arthanzel and ran the tests. I installed nyc to get the coverage details from mocha:

--------------------- --------- ---------- --------- --------- --------------------------------------- File % Stmts % Branch % Funcs % Lines Uncovered Line #s
All files 92.74 87.62 87.93 92.7
bin 79.36 73.07 37.5 79.36
cli.js 79.36 73.07 37.5 79.36 ...08-111,149,158,162,165,181,187,204
examples 100 100 100 100
functions.js 100 100 100 100
list.js 100 100 100 100
output-function.js 100 100 100 100
lib 94.33 89.01 96 94.29
ejs.js 95.65 90.99 100 95.63 ...17,264,388-395,471,477,608,662-666
utils.js 89.58 80.39 91.66 89.36 200-207,240,270-272,280
--------------------- --------- ---------- --------- --------- ---------------------------------------

The lines for the new function cover from 211-312 which the groups not covered are because the tests we currently have do not utilize the full function of the library.

In my opinion this should show that no additional tests are required for the changes as the existing tests cover them. This was ran with Node 14, NPM 6 (as the lock file was version 1) - 152 passing (771ms)

mde commented 2 years ago

Jake is used for the args parsing of the CLI functionality. (There is nothing in EJS "copy pasted from Jake.") We have precisely six tests for CLI inputs. Measurements from coverage tools are not going to tell you all the supported input scenarios for which EJS has no tests, but Jake does. Jake has a massive test suite which exercises a vast number of input scenarios.

I'm happy to work on this, and happy to remove Jake when there I have sufficient test-based confidence that we won't break something. "We'll do tests later" is unfortunately an all-too commonly seen approach, and not one I am happy to indulge in.

The amount of extra code we're talking about installing via NPM is frankly tiny, and the Jake dependency is not even shipped to the browser in any case, so I will prioritize safety over expediency here, and thank you all in advance for your patience.

hppycoder commented 2 years ago

@mde Will you please have a new release then as per the PR - https://github.com/jakejs/jake/pull/411. This includes the latest version which will also resolve the CVE against async. After you make an official release there, please make another on this project bumping the dependency.

shimonbrandsdorfer commented 1 year ago

This will allow us to address #690 as well