Allow semver ranges #2011
Allow semver ranges #2011
Conversation
|
Dappnode bot has built and pinned the built packages to an IPFS node, for commit: e9e20dc This is a development version and should only be installed for testing purposes.
Hash: (by dappnodebot/build-action) |
| * @param semverRange The semver range to be parsed. | ||
| * @returns The parsed version as a string. | ||
| */ | ||
| private parseSemverRangeToApmVersion(semverRange: string): string { |
There was a problem hiding this comment.
For context:
- The current package manifest version pattern is
"pattern": "^((([0-9]+).([0-9]+).([0-9]+)))$",- The repository SC only allows strict semver, see
function isValidBump(uint16[3] _oldVersion, uint16[3] _newVersion) public pure returns (bool) {
bool hasBumped;
uint i = 0;
while (i < 3) {
if (hasBumped) {
if (_newVersion[i] != 0) {
return false;
}
} else if (_newVersion[i] != _oldVersion[i]) {
if (_oldVersion[i] > _newVersion[i] || _newVersion[i] - _oldVersion[i] != 1) {
return false;
}
hasBumped = true;
}
i++;
}
return hasBumped;
}Which means that we will kind of follow JS ecosystem where packages will have always a semver version and the package manager can parse the dependency target defined in the manifest (package.json)
There was a problem hiding this comment.
we should document this in the dev documentation letting devs know that they are allowed to do so
| } | ||
|
|
||
| // Use x.x.x if version is ^x.x.x or ~x.x.x | ||
| if (/^[\^~]\d+\.\d+\.\d+$/.test(semverRange)) { |
There was a problem hiding this comment.
is this correct? shouldnt determine the following?
^1.2.3 - Matches the latest patch version for the 1.2.x range.
~1.2.3 - Matches the latest minor version for the 1.x.x range.
There was a problem hiding this comment.
This has been fixed in 2b8c470
| * @param semverRange The semver range to be parsed. | ||
| * @returns The parsed version as a string. | ||
| */ | ||
| private parseSemverRangeToApmVersion(semverRange: string): string { |
There was a problem hiding this comment.
I would rename the function to something like parseSemverRangeToSemverStrict since semver is something not specific to APM
There was a problem hiding this comment.
This function has been removed, as it is not needed anymore
| * @param dependencies The main dependencies object to be processed. | ||
| * @param installedPackages The list of currently installed packages. | ||
| */ | ||
| private filterSatisfiedDependencies( |
There was a problem hiding this comment.
It seems to me weird deleting the dependency if its installed already. It must be checked still if it satisfies the version range defined
There was a problem hiding this comment.
I would take a different approach than deleting the dependency, it is still a dependency so whenever its going to be installed that code should bypass if already installed and version satisfies
| } | ||
| } | ||
|
|
||
| private async defineExactVersions( |
There was a problem hiding this comment.
could you add a description to the function also providing an example of how looks the object before and after?
|
This should not be necessary as it is already supported in another way |
This PR refactors the
DappGetFetcherclass to improve the handling of dependencies and their version ranges. The main improvements include:Separation of Concerns:
processDependenciesmethod was introduced to handle two main responsibilities:Improved Optional Dependencies Handling:
mergeOptionalDependenciesmethod was introduced to inject optional dependencies into the main dependencies object only if the corresponding packages are installed.Enhanced Semver Range Handling:
parseSemverRangeToApmVersionmethod was created to convert semver ranges like^x.x.x,~x.x.x,>x.x.x, and>=x.x.xto appropriate APM-compatible versions or the latest version (*) if applicable.Code Readability and Maintainability: