isomorphic-git / isomorphic-git

A pure JavaScript implementation of git for node and browsers!
https://isomorphic-git.org
MIT License
7.47k stars 375 forks source link

Any Plans for DenoJS support #1297

Open jeffrey-l-turner opened 3 years ago

jeffrey-l-turner commented 3 years ago

Are there any plans for DenoJS support? If not, I'd like to volunteer figuring out how to get it in.

jcubic commented 3 years ago

Please go ahead and try to make it work with Deno. If the code doesn't break the code and tests it may be merged into the project.

aiotter commented 2 years ago

While I am trying to make it work in deno, I got an error. It is probably due to a bug in deno std module (std@0.119.0/node/_buffer.js), or isomorphic-git itself.

import * as fs from "https://deno.land/std@0.119.0/node/fs.ts";
import * as git from "https://cdn.skypack.dev/isomorphic-git@1.10.3?dts";

const commitOid = await git.resolveRef({ fs, dir: ".", ref: "HEAD" });
const obj = await git.readCommit({
  fs,
  dir: ".",
  oid: commitOid,
});
// ^ Got error here
// Length mismatch: expected 49,54,53 bytes but got 165 instead.

While executing fs.read(), Buffer.prototype become Uint8Array, so Object.setPrototypeOf(buffer, Buffer.prototype); made buffer is an instance of Uint8Array. https://github.com/denoland/deno_std/blob/14ee4b289f2854453e52cb3bcf4f329bfc47208a/node/_buffer.js#L862

This is why you get length as 49,54,53 after const length = buffer.slice(s + 1, i).toString("utf8");, which causes the error. https://github.com/isomorphic-git/isomorphic-git/blob/72b3987c29817392360cba80407f6d4625f5ba77/src/models/GitObject.js#L15

I'm not sure which to blame for this, deno std library or isomorphic-git. Both modify Buffer.prototype. If anyone has good idea for fundamental fix, please share it with me.

For a time being, const length = Array.from(buffer.slice(s + 1, i)).map((i) => String.fromCharCode(i)).join(); or something will solve this as a quick fix. I'll create a PR for this later.

mojavelinux commented 2 years ago

Both modify Buffer.prototype.

To my knowledge, isomorphic-git does not modify Buffer.prototype. Where do you see this? It does, however, use a Buffer proxy called BufferCursor. Are you suggesting there is an issue with that proxy?

aiotter commented 2 years ago

Sorry, it seems to be my misunderstanding. I created a small code without isomorphic-git and still see Buffer.prototype is Uint8Array. (I traced the whole process on DevTools on Chrome browser. It was a long journey, and I seems to mix up modules)

aiotter commented 2 years ago

I made additional research and found on both node.js and deno Buffer.prototype is Uint8Array at runtime. It seems that deno just implements Buffer.toString() in incompatible way to node.js.

aiotter commented 2 years ago

I'm so sorry that I was completely messed up. It wasn't a problem of neither deno nor isomorphic-git. It was due to skypack.dev CDN, which is responsible for converting node.js APIs to browser ones. Switching to esm.sh, which converts NPM packages to ES modules, solved the problem like a magic.

Those who wants to make isomorphic-git work in deno can code like this way. Easy job.

import * as fs from "https://deno.land/std@0.119.0/node/fs.ts";
import * as git from "https://esm.sh/isomorphic-git@1.10.3";

const commitOid = await git.resolveRef({ fs, dir: ".", ref: "HEAD" });
const obj = await git.readCommit({
  fs,
  dir: ".",
  oid: commitOid,
});
console.log(obj);

This issue can be closed IMAO, at least as long as they run esm.sh CDN.

How about mentioning how to run on deno at README?

jcubic commented 2 years ago

@aiotter I would keep this issue open and update documentation. Maybe README with information that isomorphic-git work with Deno. Do you want to contribute? Just add this info + your code to README, maybe in getting-started section.

aiotter commented 2 years ago

@jcubic Nice, I'll create a PR later then.

kayac-chang commented 2 years ago

Hello, I create a simple example for this issue, work well for me.

import git from "https://esm.sh/isomorphic-git@1.17.2";
import http from "https://esm.sh/isomorphic-git@1.17.2/http/node.js";
import fs from "https://deno.land/std@0.140.0/node/fs.ts";
import path from "https://deno.land/std@0.140.0/node/path.ts";

await git.clone({
  fs,
  http,
  dir: path.join(Deno.cwd(), "test-repo"),
  url: "https://github.com/isomorphic-git/lightning-fs",
});

console.log(await Deno.realPath("./test-repo"));
jcubic commented 2 years ago

@kayac-chang the issue is already resolved, there is only a need to update documentation.

danielr1996 commented 2 years ago

Any updates on this? I understand that it works as @kayac-chang showed, but what is more important is: if someone is planning to use isomorphic-git with deno (like I am), is there any "official" support for running it on deno (as the name 'isomorphic' suggests it should be support, because after all its just javascript that doesn't depend on runtime and thats the whole point of this library, right?). By official I don't mean test every edge case upfront and guarantee that it works the same as on node or in the browser, but for me and probably others it would be important that issues regarding deno would be taken serious and not be just closed with the message "your problem, this library doesn't support deno".

So what are your thoughts on this?

jcubic commented 2 years ago

@danielr1996 If this is how you understand support, then Deno will be supported if there will be people willing to fix Deno bugs. This project doesn't have the main author that writes all the code for free and it's driven by contributors. I'm a maintainer but I'm only doing CR and merges, I don't write any code myself. At least not recently. So if you be willing to help with Deno when you will have any issues then Deno will be supported. But don't expect that someone will fix your problems with Deno. Another idea is to find a developer that is willing to fix any Deno issues. We have some money from Open Collective so we can even give some to such a person. As of now, we don't spend money on anything. The problem is finding such a person.

jcubic commented 2 years ago

I can assure you that no one will close Deno's issues as out of scope. The library works in Deno so it should be added to documentation and can be official. I would love if someone updated unit tests so they run on Deno as well as NodeJS so we'll be sure that everything is working.

danielr1996 commented 2 years ago

That's great to hear. Regarding unit test: Deno has it's own test runner thats build into the cli itself which makes it nearly impossible to execute the same test on both runtimes. I'd really like to take a look at them, but I have no idea how we could run the same tests on both runtimes.

jcubic commented 2 years ago

Is deno built-in test the only one, I hope you can run jest inside Deno, it would be pretty useless if it doesn't support this library and force to use Deno solution for unit tests.

danielr1996 commented 2 years ago

I have bad news for you, Deno has just its builtin test runner and some deprecated libaries that all use Deno.test() under the hood. Unfortunatelly every resource online tells you "just use Deno.test(), it's builtin, no need for jest" but Deno.test() doesn't even come close when it comes to jest, especially regarding coverage, mocking and IDE integration. So Deno doesn't force it's internal library, there has just been no interest in writing a 3rd party solution.

jcubic commented 2 years ago

I would just try to run tests as they are right now, but I'm not sure if they run locally even in Node.js. There was issues created for this, I was not able to run them myself locally.

You can experiment with CI/CD and add testing with deno and check the errors, if tests don't run locally this may be the only way to check if tests work on Deno.