-
Notifications
You must be signed in to change notification settings - Fork 7
NPM packages live update #717
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
Conversation
|
Here is a quick run of the new > brioche run -e liveUpdate -p packages/aws_cdk
Build finished, completed (no new jobs) in 5.99s
Running brioche-run
{
"name": "aws_cdk",
"version": "2.1019.1",
"extra": {
"packageName": "aws-cdk"
}
} |
…ages Signed-off-by: Jérémy Audiger <[email protected]>
…ck new release of NPM packages Signed-off-by: Jérémy Audiger <[email protected]>
ba77a9d to
3bbcb3a
Compare
|
To be merged after #716, and once the comments in the PR description are addressed |
kylewlacy
left a comment
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.
LGTM!
Left some notes that might be good to do as a follow-up. I'm happy to merge as-is since 1) it makes the code better now and 2) I like the idea of testing it end-to-end during the next batch of live updates
| */ | ||
| interface LiveUpdateFromNpmPackagesOptions { | ||
| project: { version: string; extra: LiveUpdateFromNpmPackagesExtraOptions }; | ||
| matchVersion?: RegExp; |
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.
...any idea how useful this option will be? I don't think I've ever seen an NPM package that didn't use an x.y.z style version number (unlike git tags, where we already have several packages with weird tag names). I also haven't looked into NPM's rules to see what they allow for versions, I might try and do some research on this as a follow-up....
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 didn't look neither for the NPM rules related to versioning schema. I was thinking it could be worth it to have it here, since it wasn't that complicated to re-use (since the code was already here from GitHub releases).
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.
Well, I just quickly checked. NPM versioning is following the Semantic versioning Schema. So, the version cannot be prepended with v, but metadata could be added after Z
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 is no need indeed to have the matchVersion field here if SemVer is respected.
This PR refactors multiple project files to improve maintainability and standardize the implementation of live updates for NPM packages. It introduces a new utility,
std.liveUpdateFromNpmPackages, replacing custom implementations ofliveUpdateacross various packages. Additionally, it consolidates version-matching logic into a shared constant and improves documentation for related utilities.I took this opportunity to move all the expected live-update fields of a NPM package to extra:
This is a bit different than the GitHub live update approach where the live-update fields are directly at the project root. But it makes sense here since we're talking about a repository URL.
Another difference between
std.liveUpdateFromGithubReleasesandstd.liveUpdateFromNpmPackagescomes from the options interface:We have on one side a
matchTagand on the other side amatchVersion, at the end we are talking about version. even when we look at the GitHubversion/tagextraction we are using both terminologies:So, I also updated the name of the constant regex to be
DEFAULT_LIVE_UDPATE_REGEX_VERSION_MATCH, but I didn't update the interfaceLiveUpdateFromGithubReleasesOptions, what do you think @kylewlacy should we uniformize all of that ?