Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use execa #115

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

feat: Use execa #115

wants to merge 3 commits into from

Conversation

MichaelCurrin
Copy link
Owner

@MichaelCurrin MichaelCurrin commented Nov 4, 2024

Relates to #114

The new approach seems to be okay except for errors in running - it complains about the modules.

Switching package.json to type module and tsconfig to esnext did not help as that caused other issues in the build/compile step.

image

@@ -29,7 +26,7 @@ function _execute(cwd: string, subcommand: string, options: string[] = []) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, for the array in QUOTE_PATH to help with escaping you would need to use execa's array syntax (which is the concept I was referring to in the issue)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I did a standalone test using array and nesting it.

But forgot to do it here, I guess on line 28 for command as array rather than strings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, (besides git needing to be extra). Line 29 could look like this as far as I see it:

return execa("git", [...QUOTE_PATH, subcommand, ...options] ,{ cwd });

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. I added #116 as a task to update to module type and then this PR will be possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants