-
Notifications
You must be signed in to change notification settings - Fork 1
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
[88] Add version script to build tools #119
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting the work into this Jordan. There's a lot to understand within this process and Node.js has its own quirks that you don't get in the browser.
I've left some feedback, there's a few issues that I've noticed. Some minor, and some that will need more work. I've also got some additional issues that I found through testing that can't be added as part of the code review so I'll make individual comments.
src/commands/version.js
Outdated
// with no projects listed it returns a empty array | ||
const projectsList = projects.split(',').filter((item) => item.length > 0); | ||
const excludeDirs = ['node_modules', 'dist', 'build', 'vendor', 'vendor_prefixed']; | ||
const targetFiles = ['package.json', 'style.css', 'plugin.php']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are additional target files and it wouldn't just be limited to those three. If you refer back to our Slack conversation it was mentioned that there could be additional plugin files that we would need to look at.
Much like using plugin.php
as the entry point file for a plugin file, it's also possible to use any other named file. As long the heading doc block exists. Normally these will be the same as the plugin directory but can be different, there can also be multiple.
As an example the following are all valid in any combination and/or multiple:
File | Works |
---|---|
example-plugin/plugin.php |
✅ |
example-plugin/example-plugin.php |
❌ |
example-plugin/some-file.php |
❌ |
example-plugin.php |
❌ |
Because of this it doesn't make sense to have a hardcoded list, and should be more dynamic to account for this variation in possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored this now so it searches files for Version instead of finding specific files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come across several issues with this change, you can find them discussed in this comment.
src/commands/version.js
Outdated
/** | ||
* Function to search for all files which are not in the excluded directories | ||
* | ||
* @param {string} basePath - The base directory to start searching from. | ||
* @param {Array} directoryNames - An array of directory names to match. | ||
* @returns {Array} - An array of matching directory paths. | ||
*/ | ||
const getAllFiles = (dirPath, arrayOfFiles) => { | ||
const files = fs.readdirSync(dirPath); | ||
|
||
arrayOfFiles = arrayOfFiles || []; | ||
|
||
files.forEach((file) => { | ||
const filePath = path.join(dirPath, file); | ||
if (fs.statSync(filePath).isDirectory()) { | ||
if (!excludeDirs.includes(file)) { | ||
arrayOfFiles = getAllFiles(filePath, arrayOfFiles); | ||
} | ||
} else { | ||
arrayOfFiles.push(filePath); | ||
} | ||
}); | ||
|
||
return arrayOfFiles; | ||
}; | ||
|
||
/** | ||
* Function to find the files we want to use. | ||
* @param {string} baseDirs - Base directory to search from. | ||
* @param {Array} targets - Files we want to find | ||
*/ | ||
const findTargetFiles = (baseDirs, targets) => { | ||
baseDirs.forEach((baseDir) => { | ||
if (fs.existsSync(baseDir) && fs.statSync(baseDir).isDirectory()) { | ||
const allFiles = getAllFiles(baseDir); | ||
allFiles.forEach((filePath) => { | ||
const fileName = path.basename(filePath); | ||
if (targets.includes(fileName)) { | ||
incrementVersionNumber(fileName, filePath, type); | ||
} | ||
}); | ||
} else { | ||
console.error(`Directory not found: ${baseDir}`); | ||
} | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to recursively seek out files in this manner. This will add a lot of overhead when creating compiling the list, especially in larger projects where theres multiple layers and directories. You can reduce this down into something more performant by using the project directories and targeting those, as we already know that all the files required will live in the project directory.
For example these are what you need to be looking for at a minimum:
File |
---|
example-plugin/*.php |
example-plugin/style.css |
example-plugin/package.json |
example-plugin/package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ampersarnie would it be possible to get another example on this please im struggling to get my head around this. My thoughts are that you would need to recursively search to find the plugins ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the build
command file you will see that it already holds functionality for finding plugins/themes that Build Tools can and should process. Lines 62 through to 113 handle all of the looking up projects and creating that list for you to work from. It provides a whole range of paths and file points that you can use, such as the package.json
.
Because we know that the files you need to increment are in the root of a project (i.e. plugins/my-plugin/*
), you can target files within that directory alone, removing the need for recursion.
For consistency with targeting correct projects, you should use that same process that the build
command uses, rather than trying to create it from scratch and implementing a different solution.
As for why not use recursion, there are a number of them:
- It can add unnecessary processing to the file look up as its not just targeting files, or directory levels that we are aware of (the root level). But it's also going to look through every other directory in the plugin. For example I was able to get a file processed that I added to
dir1/dir2/dir3/some-file.php
. It doesn't need it for the case we have here. - In conjunction with the changes you made here and after further testing, it is also possible due to the recursion setup to have a plugin and/or theme that should not be processed by Build Tools. This is because the recursion and lookup is targeting any file and any directory that has a
package.json
file. This is going to cause issues with updating versions in 3rd party plugins/themes. Take the following steps:
- Create a plugin package.json -
plugins/non-build-tools-plugin/package.json
- and add the following basic config:
{
"version": "12.3.4"
}
- Create a plugin entry file -
plugins/non-build-tools-plugin/plugin.php
- and add the following:
<?php
/**
* Version: 12.3.4
**/
- Run a version update on all projects from
wp-content
build-tools version patch
- You should see
non-build-tools-plugin
being processed in the list.
That dummy project you have created should not have a version increase as it doesn't have the correct setup to work across our build setup. You will also see the same results if you add a third-party plugin/theme to one of the related directories - client-mu-plugins
, plugins
or themes
.
Does this help?
Not able to update from project directory.When working from a theme or plugin directory, I am not able to increment the version of that project. I should be able to increment a project version on an individual basis. Example
Output
Note Edit: Issue has been addressed. |
Short version numbers not updated.When updating a theme or plugin, if there is an assignment of a version such as
Example
The blow is output, where it can be seen that it does not pick up the changed
Note Edit: Issue has been addressed. |
|
There are no options for a Release CandidateNot in the original ticket but it would be nice to be able pass an Example
Output
Note Edit: Issue has been addressed. |
Messaging inconsistenciesTerminal output doesn't have consistent approaches and language. The following changes should be made:
Note Edit: Issue has been addressed. |
non-lowercase subcommands failUsing subcommands that are not lowercase will fail, causing the same Example
Output
You have two options on how to approach this:
Note Edit: Issue has been addressed. |
Spacing inconsistenciesThere's a fair amount of spacing consistencies between variable, and logical declarations. There are some indentation issues across files that would need addressing also. const example = 'value';
if (logicCheck !== 123) {
// ...
} Keeping some separation will help readability overall for others. |
Terminal Output ColoursIt would be nice to be able to have some additional styling and formatting with the terminal output that takes advantage of ANSI escape codes and colours. See how it is done here for example. Note Edit: Issue has been addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following issues still need addressing:
- There are no options for a Release Candidate - [88] Add version script to build tools #119 (comment)
- Spacing Inconsistencies - [88] Add version script to build tools #119 (comment)
@ampersarnie on this comment #119 (comment) Could you let me know what im missing here please? when I add a RC to the command it adds a rc release number http://bigbite.im/i/qcTpJF Also I added the 1 to the prerelease to semver inc as requested however it still comes out as .0 I wondered if you know where im going wrong with this? Thanks 😄 |
When I'm looking at it, its increasing the wrong version. So if I have a current version of
I changed it on your previous version to a
|
@ampersarnie I think I have solved this by removing an if statement and adding in another command http://bigbite.im/v/ZIup9l You can now do build-tools version prerelease:beta << this will change the last value in the version after -beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not done any thorough testing this time around, this is purely a code review. Will test once these have been addressed.
src/commands/version.js
Outdated
const { getPackageVersion } = require('./../utils/get-package-version'); | ||
const { findAllProjectPaths } = require('./../utils/get-version-project-paths'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need these custom functions, we already have prior art that covers both of these situations. As mentioned in this comment you can use the same setup from the build
command file.
If you look in the build command file, and at lines 62 through to 113 you will see the function in use that compiles the list of packages available to compile. There you will also find the code that extracts a object config which creates all variations of the paths required for you to use these, including a path to the package.json
file and extracted version from that file.
To keep things consistent across both commands, use these and adapt them if you need.
src/commands/version.js
Outdated
try { | ||
if (projectsList.length === 0 && !isAllProjects) { | ||
// Increment projects in current directory | ||
terminal('\x1b[1mIncrementing Projects In \x1b[4mDirectory\x1b[0m...\n'); | ||
packages.push(getPackageVersion(path.resolve('./'))); | ||
} else if (isAllProjects) { | ||
// Increment all projects i.e from wp-content folder | ||
terminal('\x1b[1mIncrementing \x1b[4mAll Projects\x1b[0m...\n'); | ||
packages = findAllProjectPaths(targetDirs).map((path) => getPackageVersion(path)); | ||
} else { | ||
// Increment specified projects | ||
terminal('\x1b[1mIncrementing \x1b[4mList of Projects\x1b[0m...\n'); | ||
packages = findAllProjectPaths(targetDirs, projectsList).map((path) => | ||
getPackageVersion(path), | ||
); | ||
|
||
const packageNames = packages.map((pkg) => pkg.name); | ||
projectsList.map((projectName) => { | ||
if (!packageNames.includes(projectName)) { | ||
terminal.red(`Error: Project ${projectName} does not exist.\n`); | ||
} | ||
packageNames.includes(projectName); | ||
}); | ||
} | ||
} catch (e) { | ||
terminal.red(e); | ||
process.exit(1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're creating a lot of duplicate code here, as mentioned in my other command you can use what exists in the build
command. You should extract those functions and processes out into their own files, so you have shared utils.
src/utils/get-package-version.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this, see previous command about removing it and using prior art/code from build
command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this, see previous command about removing it and using prior art/code from build
command.
@ampersarnie I've refactored this now to re-use the existing functions |
88 - This PR adds a new command to the build tools package so you can increase the increment the project version in all files without updating them manually.
The command is
build-tools version <version type> <project>
The versions currently are
*X representing the value which will change when the command is applied.
The project command can be left blank which will search the whole directory or you can list projects for example theme,plugin1, plugin2 etc
This will then go through and update the files in those directories.
It currently updates the version in package.json, plugin.php and style.css
To test:
Once you have build-tools and yalc added to your test project
Run
build-tools version minor
for example and this will go through all the files and folders in the directory your in and increment the version.It will also return which files it found and inform you of the old version and the version it has been upgraded too.
Video -> http://bigbite.im/v/tf2VBx
Change Log