search

loreanvictor / tmplr-node

Node.js bindings for tmplr
MIT License
0 stars 1 forks source link

Switched to using the `tiged` module instead of the `degit` module. #2

Closed Morgul closed 4 months ago

Morgul commented 4 months ago

This change switches to using tiged in place of degit, to fix issues with cloning from gitlab.

This should fix https://github.com/loreanvictor/tmplr/issues/28.

Added types from @types/degit, ported to match tiged's api. (Note: There's an open PR porting tiged to TypeScript, so these types won't be required forever.)

loreanvictor commented 4 months ago

@Morgul I think some tests need to be updated. additionally, degit's issue with subgroups are supposed to be fixed now (?). should we still replace degit with tiged?

Morgul commented 4 months ago

So, the issues with the tests are cause because I forked the project, so for this test, it's getting a different value:

image

I didn't think updating it made since, as it would break the main repo once it was merged. I'm happy to update the test, just not sure the best way to update. Maybe we just skip getting the repo owner as part of the test?

As for degit: I still can't use tmplr with subgroups on gitlab, so I think the change is still needed. It hasn't had any commits in 3 years and I think most development is happening over at tiged anyway.

loreanvictor commented 4 months ago

So, the issues with the tests are cause because I forked the project, so for this test, it's getting a different value ... I'm happy to update the test, just not sure the best way to update. Maybe we just skip getting the repo owner as part of the test?

Well then we wouldn't be testing that functionality 😅 Yeah this is my bad because I didn't think of forks when writing this particular test. I suppose the cleanest solution would be to create a temporary folder (this is already done in the next test), clone a pre-designated repository inside it, and then create a git provider within that repository and check the values accordingly, so the test is not dependent on the current repository.

If fixing this issue is a hassle though, I suppose it is ok to ignore the test right now and issue a fix for it later, since it wouldn't fail for the actual build of the library.

Also would it be possible to add a test highlighting the issue you had? e.g. using node-fs to clone a repository that couldn't have been cloned before?

Morgul commented 4 months ago

If fixing this issue is a hassle though, I suppose it is ok to ignore the test right now and issue a fix for it later, since it wouldn't fail for the actual build of the library.

I'll give it a shot, I should be able to work that out. I was mainly trying to have as minimally invasive a set of changes as possible.

Also would it be possible to add a test highlighting the issue you had? e.g. using node-fs to clone a repository that couldn't have been cloned before?

...Ok, this one's my bad. I should have added a test for that! Probably won't be today, but I'll circle back around and get this updated.

Morgul commented 4 months ago

I'll give it a shot, I should be able to work that out. I was mainly trying to have as minimally invasive a set of changes as possible.

Pushed a change that makes a temp directory and does a checkout. I went with the simplest option, which is just using child-process to call git; I wasn't sure if you'd rather add something like node-git or how you'd want to handle it, but it seems to work well.

I'll work on writing the new test next, figured I'd get this change in first so you could look at it and give me feedback if you want changes.

loreanvictor commented 4 months ago

I'll give it a shot, I should be able to work that out. I was mainly trying to have as minimally invasive a set of changes as possible.

Pushed a change that makes a temp directory and does a checkout. I went with the simplest option, which is just using child-process to call git; I wasn't sure if you'd rather add something like node-git or how you'd want to handle it, but it seems to work well.

I'll work on writing the new test next, figured I'd get this change in first so you could look at it and give me feedback if you want changes.

the code looks good to me. I was wondering whether there the OS-dependent quirks of childProcess() would affect the test in different environments but it appears not (as all tests are passing correctly in all envs).

Morgul commented 4 months ago

@loreanvictor Sorry it's taken me a minute to finish this up.

I've got a test all worked up for the new functionality, but I've hit a snag I missed previously; it looks like you need to pass tiged a subgroup: true option to make it work correctly with subgroups. Doing that works well, but breaks anything not a subgroup.

Do you think it's worth adding support for setting tiged options through the fetch function? Or maybe just adding a single optional subgroup parameter to control the required tiged option?

loreanvictor commented 4 months ago

@loreanvictor Sorry it's taken me a minute to finish this up.

I've got a test all worked up for the new functionality, but I've hit a snag I missed previously; it looks like you need to pass tiged a subgroup: true option to make it work correctly with subgroups. Doing that works well, but breaks anything not a subgroup.

Do you think it's worth adding support for setting tiged options through the fetch function? Or maybe just adding a single optional subgroup parameter to control the required tiged option?

what are other tiged options? I suspect we could add an option for subgroups, up-propagating it to tmplr CLI itself. it's not really neat, but it might be ok.

Morgul commented 4 months ago

For now, I exposed the subgroup option. The other options seem like renamed versions of the degit options, so it's probably not worth exposing all of them right now.

loreanvictor commented 4 months ago

as a reminder for future, and to track progress, putting this on production requires:

Morgul commented 4 months ago

Thanks for that checklist, it means (as I have time) I can work on the various parts of this to get it across the line!