Nodegit should be an optional peer dependency

[Adjective-Object]

When nodegit fails to install (e.g. on node 16+ on a system without the dependencies for libgit2), requiring good-fences at all fails with this require stack:

  requireStack: [
    'node_modules/nodegit/dist/nodegit.js',
    'node_modules/good-fences/lib/utils/diffing/getFenceAndImportDiffsFromGit.js',
    'node_modules/good-fences/lib/core/runner.js',
    'node_modules/good-fences/lib/index.js',
  ]

This is because we import nodegit even when it is not being used.

We can work around this by making it an optional dependency and importing the module only when partial diff functionality is actually used (e.g. runtime require())

[Adjective-Object]

Should we consider migrating to wasm-git instead?

[gustaff-weldon]

@Adjective-Object @smikula if switching to wasm-git bodes better when it comes to compilation issues, please consider moving on to it. The nodegit compilation errors are plaguing our CI and docker setup. We had to enforce alpha nodegit release via yarn resolutions to be able to install good-fences on Macs (x86). Now, we started getting errors on alpine linux with Node 16...

[tianhuil]

@smikula: do you know if there's any progress on this? We would love to use good-fences, but the node-git issues are causing issues installing on our Github CI and keeping us from using good-fences.

[smikula]

@Adjective-Object, is this something that would be easy to take care of? I know we're not really using this version of good-fences anymore, but I'd like to make sure it remains useful for other people.

@tianhuil, if you aren't aware, we've reimplemented good-fences in rust for a much faster experience. You might find that moving to that solves your problem and gets better performance.

[tianhuil]

Thank you @smikula! Should you put a warning on the README.md if that's the recommended version?

[gustaff-weldon]

Thank you @smikula! Should you put a warning on the README.md if that's the recommended version?

@smikula I second that, I was not aware there's a reimplementation also distributed via npm, so looks like an easy replacement .

Are you going to add support in the new version for blacklisting, ie. listing dependencies that module is not allowed to use? That would be very useful for cases where we want to prevent people from importing certain code we do not want them to rely on.

[tianhuil]

The nodegit issue can be solved by setting

{ 
  "overrides": {
	"nodegit": "npm:[email protected]"
  }
}

See https://github.com/microsoft/FluidFramework/pull/15572 . Credit due to @CraigMacomber

[wawyed]

Any updates on this?