fix: Replace parse-git-config with simple-git #1485
Conversation
Migrate from `parse-git-config` to `simple-git` library due to lack of support and security vulnerabilities. * **Replace library in `get-repo-slug.ts`** - Remove import statement for `parse-git-config` - Add import statement for `simple-git` - Replace usage of `parse-git-config` with `simple-git` to retrieve repository information * **Remove module declaration in `ambient.d.ts`** - Remove the module declaration for `parse-git-config` * **Update dependencies in `package.json`** - Remove `parse-git-config` dependency - Add `simple-git` dependency --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/danger/danger-js?shareId=XXXX-XXXX-XXXX-XXXX).
ryanb93
force-pushed
the
replace-parse-git-config
branch
from
046a76d to
ee62071
Compare
ryanb93
changed the title
| resolved "https://registry.yarnpkg.com/simple-git/-/simple-git-3.27.0.tgz#f4b09e807bda56a4a3968f635c0e4888d3decbd5" | ||
| integrity sha512-ivHoFS9Yi9GY49ogc6/YAi3Fl9ROnF4VyubNylgCkA+RVqLaKWnDSzXOVzya8csELIaWaYNutsEuAhZrtOjozA== | ||
| dependencies: | ||
| "@kwsites/file-exists" "^1.1.1" |
There was a problem hiding this comment.
Concerning dependency: no-commits in 5 years https://github.com/kwsites/file-exists & similarly to the promise-deferred library (see next line).
simple-git as a whole hasn’t had any commits in 4 years https://github.com/simple-git-js/simple-git/
It’s one thing if tools are based in a mature stack that doesn’t change. But all these tools are based on node & npm. And as we all now, that stack has been constantly changing. I’m not sure going from a package that last saw maintenance 6 years ago to one that is 4 years ago is a big enough improvement? (Also the debug library is a little weird)
My 2cents: I’d expect new libraries we adopt have a story about ESM (& CJS still), otherwise we’re piling up more work in front of maintaining danger-js in the next year or so.
There was a problem hiding this comment.
That's not the dependency: https://www.npmjs.com/package/simple-git is https://github.com/steveukx/git-js
There was a problem hiding this comment.
I’m much less worried about simple-git now, thanks @orta! My questions about the transitive deps are still there, but active maintenance on simple-git is at least reassuring
There was a problem hiding this comment.
For me, the biggest flaw in this PR is that parse-git-config did one simple thing in 200 lines - this new dependency is massive in comparison because it implements every git command and everyone has to get all this stuff so we can just look at a list of remotes in danger init
Why did you choose it? Did you look for a smaller dependency @ryanb93 ?
There was a problem hiding this comment.
Honestly, it just came up as the first library that could implement this functionality - but you're correct, it's a heavy dependency to bring in for only a single task. I'm not sure how you feel about using execSync but we could remove these dependencies entirely and use something like:
const remoteUrl = execSync('git config --get remote.origin.url').toString().trim();There was a problem hiding this comment.
Yeah, I think that's pretty reasonable 👍🏻
|
superseded by #1486 |
Fixes #1482
Migrate from
parse-git-configtosimple-gitlibrary due to lack of support and security vulnerabilities.Replace library in
get-repo-slug.tsparse-git-configsimple-gitparse-git-configwithsimple-gitto retrieve repository informationRemove module declaration in
ambient.d.tsparse-git-configUpdate dependencies in
package.jsonparse-git-configdependencysimple-gitdependencyFor more details, open the Copilot Workspace session.