wp-env: Use Simple Git instead of NodeGit #28848
Conversation
|
Size Change: 0 B Total Size: 1.38 MB ℹ️ View Unchanged
|
|
Seems to work locally, and in CI 🎉 I'm still getting a few |
|
Please let me know of any adjustments I will need to make to the Block Editor documentation for setting up a local WP dev environment: https://developer.wordpress.org/block-editor/tutorials/devenv/ |
Will do! I don't expect many user-facing changes, except we might lose the progress meter while wp-env is cloning git repos. |
|
It seems that #28741 is no longer blocked by this PR. It'd still be nice to get it in eventually (once the |
|
@ockham Looks like we have to require Seeing if that fixes the |
|
Part of the problem might be that we have a different version of it in packages/wp-env than we do at the root 🤔 |
noahtallen
force-pushed
the
update/wp-env-use-simple-git
branch
from
0b63d7b
to
ced18c7
Compare
Yep, after addressing this, everything is working correctly. However, |
|
Ok, so yeah, that definitely caused a lot of problems. I'm thinking we should do the update and fix the commands (probably not too hard) in Alternatively, we could stay on a legacy version, and only utilize legacy-version-compatible syntax for wp-env. Otherwise, this is testing pretty well for me. 👍 |
|
Thanks for tracking this down, Noah! 🎉
Yep 👍 On a sidenote,
I'd rather use the new version, especially since I think we'll see
🥳 |
|
Looking at the CI errors and https://github.com/steveukx/git-js#usage, I think the fix might be as simple as diff --git a/bin/plugin/lib/git.js b/bin/plugin/lib/git.js
index b8b4242800..9af6288b99 100644
--- a/bin/plugin/lib/git.js
+++ b/bin/plugin/lib/git.js
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-const SimpleGit = require( 'simple-git/promise' );
+const SimpleGit = require( 'simple-git' );
/**
* Internal dependencies
|
|
I'm getting a ton of TypeScript errors that look like this now (see CI): They happen on lines where I try to instantiate const SimpleGit = require( 'simple-git' );
const git = SimpleGit(); // <-- error |
|
Do we need to convert wp-env to typescript? |
I'd love to do that actually, the jsdoc types are both important and a pain to work with.
|
@ockham I added I think part of the problem is that this file is written as a node script (and therefore uses For example, |
noahtallen
force-pushed
the
update/wp-env-use-simple-git
branch
from
7176680
to
a3861aa
Compare
ockham
force-pushed
the
update/wp-env-use-simple-git
branch
from
a3861aa
to
93acbab
Compare
|
Thanks @noahtallen and @jsnajdr! The TS ignore seems like a good solution for now. Static analysis checks have been complaining about an outdated package-lock. Rebasing to attempt a fix 🤞 |
|
FWIW, I've briefly tried removing the Diffdiff --git a/bin/plugin/lib/git.js b/bin/plugin/lib/git.js
index 6bf62a0f8f..0f9c64ddbb 100644
--- a/bin/plugin/lib/git.js
+++ b/bin/plugin/lib/git.js
@@ -1,8 +1,7 @@
-// @ts-nocheck
/**
* External dependencies
*/
-const SimpleGit = require( 'simple-git' );
+import SimpleGit from 'simple-git';
/**
* Internal dependencies
@@ -16,7 +15,7 @@ const { getRandomTemporaryPath } = require( './utils' );
*
* @return {Promise<string>} Repository local Path
*/
-async function clone( repositoryUrl ) {
+export async function clone( repositoryUrl ) {
const gitWorkingDirectoryPath = getRandomTemporaryPath();
const simpleGit = SimpleGit();
await simpleGit.clone( repositoryUrl, gitWorkingDirectoryPath, [
@@ -35,7 +34,7 @@ async function clone( repositoryUrl ) {
*
* @return {Promise<string>} Commit Hash
*/
-async function commit( gitWorkingDirectoryPath, message, filesToAdd = [] ) {
+export async function commit( gitWorkingDirectoryPath, message, filesToAdd = [] ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.add( filesToAdd );
const commitData = await simpleGit.commit( message );
@@ -50,7 +49,7 @@ async function commit( gitWorkingDirectoryPath, message, filesToAdd = [] ) {
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} branchName Branch Name
*/
-async function createLocalBranch( gitWorkingDirectoryPath, branchName ) {
+export async function createLocalBranch( gitWorkingDirectoryPath, branchName ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.checkoutLocalBranch( branchName );
}
@@ -61,7 +60,7 @@ async function createLocalBranch( gitWorkingDirectoryPath, branchName ) {
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} branchName Branch Name
*/
-async function checkoutRemoteBranch( gitWorkingDirectoryPath, branchName ) {
+export async function checkoutRemoteBranch( gitWorkingDirectoryPath, branchName ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.fetch( 'origin', branchName );
await simpleGit.checkout( branchName );
@@ -73,7 +72,7 @@ async function checkoutRemoteBranch( gitWorkingDirectoryPath, branchName ) {
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} tagName Tag Name
*/
-async function createLocalTag( gitWorkingDirectoryPath, tagName ) {
+export async function createLocalTag( gitWorkingDirectoryPath, tagName ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.addTag( tagName );
}
@@ -84,7 +83,7 @@ async function createLocalTag( gitWorkingDirectoryPath, tagName ) {
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} branchName Branch Name
*/
-async function pushBranchToOrigin( gitWorkingDirectoryPath, branchName ) {
+export async function pushBranchToOrigin( gitWorkingDirectoryPath, branchName ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.push( 'origin', branchName );
}
@@ -94,7 +93,7 @@ async function pushBranchToOrigin( gitWorkingDirectoryPath, branchName ) {
*
* @param {string} gitWorkingDirectoryPath Local repository path.
*/
-async function pushTagsToOrigin( gitWorkingDirectoryPath ) {
+export async function pushTagsToOrigin( gitWorkingDirectoryPath ) {
const simpleGit = SimpleGit( gitWorkingDirectoryPath );
await simpleGit.pushTags( 'origin' );
}
@@ -115,7 +114,7 @@ async function discardLocalChanges( gitWorkingDirectoryPath ) {
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} branchName Branch Name
*/
-async function resetLocalBranchAgainstOrigin(
+export async function resetLocalBranchAgainstOrigin(
gitWorkingDirectoryPath,
branchName
) {
@@ -131,7 +130,7 @@ async function resetLocalBranchAgainstOrigin(
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} commitHash Branch Name
*/
-async function cherrypickCommitIntoBranch(
+export async function cherrypickCommitIntoBranch(
gitWorkingDirectoryPath,
commitHash
) {
@@ -146,7 +145,7 @@ async function cherrypickCommitIntoBranch(
* @param {string} gitWorkingDirectoryPath Local repository path.
* @param {string} sourceBranchName Branch Name
*/
-async function replaceContentFromRemoteBranch(
+export async function replaceContentFromRemoteBranch(
gitWorkingDirectoryPath,
sourceBranchName
) {
@@ -159,17 +158,3 @@ async function replaceContentFromRemoteBranch(
'.',
] );
}
-
-module.exports = {
- clone,
- commit,
- checkoutRemoteBranch,
- createLocalBranch,
- createLocalTag,
- pushBranchToOrigin,
- pushTagsToOrigin,
- discardLocalChanges,
- resetLocalBranchAgainstOrigin,
- cherrypickCommitIntoBranch,
- replaceContentFromRemoteBranch,
-};That almost worked, except for a small issue with That could be fixed by a type assertion: However, if I try to build that, I'm reminded that type assertions are only allowed in actual At that point, I broke off the experiment. I did file an issue against |
|
Since we'll be losing the progress meter (well, other than it showing 0% and then 100% once it's done cloning), I've also asked upstream if they could add the necessary features that might help us re-enable the progress meter later. |
|
The static analysis check is still complaining about an outdated |
|
Yeah, I also noticed that 🤔 |
FWIW, lots of fun here (and in PRs linked therein): #28834 |
|
Oh, turns out the latest version of |
I've upgraded to v2.35.0 and tried using the new progress handler in 37ccf2a, but haven't got it working (still only seeing either 0 or 100% of progress). One idea I had is that the RegEx that's looking for progress messages isn't compatible with my German locale. I tried aliasing @noahtallen Can you check if it works for you? |
There was a problem hiding this comment.
Yeah, it appears to be working for me. I will note that I had to run npx wp-env destroy to really get the full effect. I think this might just be because the progress is mostly for clone, and when fetching the latest changes (e.g. if the repos already exist), maybe there's not much progress to indicate, or it just happens:
Screen.Recording.2021-02-17.at.4.38.39.PM.mov
noahtallen
force-pushed
the
update/wp-env-use-simple-git
branch
from
37ccf2a
to
235093b
Compare
Awesome, so happy to hear it works for you, Noah! I filed an upstream bug report about this: steveukx/git-js#579 Since releases seem to happen frequently for |
|
Oh, seems like it's actually working for me now 🤔 🎉 Maybe I didn't properly clean out my |
|
Nice! |
|
@paaljoachim I don't think there is any change to the developer documentation for this PR. The issue previously was when we bumped the Node version there was not a pre-built binary for |
|
Thanks Marcus! A thank you to Bernie and Noah as well! I gave the documentation a rest for a while before I get back to it again. |
Description
Fixes #26660. Alternative to #28804.
Sincesimple-gitdoesn't expose download progress forgit clone, we lose the progress meters (i.e. they only show 0% at the start, and 100% once the download is done, but no in-between states).Edit (see #28848 (comment)): Oh, turns out the latest version of
simple-git(which was just released) now includes a proper progress handler 🥳How has this been tested?
~/.wp-envor~/wp-envfolder (warning, this removes your local WP dev install)../packages/env/bin/wp-env start. Verify that it successfully clones theWordPressandtheme-experimentsrepos, and starts a local WP install successfully.