Fix #510. Remove jake dev dependency and embed arg parser. #645
Conversation
EJS CLI introduced `jake` as a prod dependency. The only reason for this was to use jake's argument parsing code. The code was embedded into utils.js instead and jake was made into a dev dependency. This PR returns EJS to zero prod dependencies. See https://github.com/jakejs/jake/blob/master/lib/parseargs.js
|
Thanks for making progress on this, but I need tests before I can even consider merging. |
|
this will help on #659 too |
|
@mde - I pulled the branch from @arthanzel and ran the tests. I installed nyc to get the coverage details from mocha: ---------------------|---------|----------|---------|---------|---------------------------------------
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) - |
This papers over the following vulnerability: https://app.snyk.io/vuln/SNYK-JS-ASYNC-2441827 We shouldn't be impacted given we don't use the ejs CLI. mde/ejs#645
This papers over the following vulnerability: https://app.snyk.io/vuln/SNYK-JS-ASYNC-2441827 We shouldn't be impacted given we don't use the ejs CLI. mde/ejs#645
| return arg.indexOf('-') === 0; | ||
| }; | ||
| let removeOptPrefix = function (opt) { | ||
| return opt.replace(/^--/, '').replace(/^-/, ''); |
There was a problem hiding this comment.
This may not matter, but you could just use one replace call by using the regex of /^--?/ as that means that the second hyphen is optional.
There was a problem hiding this comment.
It also sorta sounds like this was just copy/pasted from jake, so I also understand this wasn't technically the op's code.
|
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. |
|
@mde Will you please have a new release then as per the PR - jakejs/jake#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. |
|
This will allow us to address #690 as well |
| arg = removeOptPrefix(arg); | ||
| argParts = arg.split('='); | ||
| argItem = this.longOpts[argParts[0]] || this.shortOpts[argParts[0]]; | ||
| if (argItem) { |
There was a problem hiding this comment.
If this condition does not meet, I. E. this is an unrecognized option, do we want to throw an error?
See #690
EJS CLI introduced
jakeas a prod dependency. The only reason forthis 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.