Update semver.js to include this.build inside of format() #684
Conversation
when you preformed format(), it was missing the this.build to be referenced. so if anyone was using this.build and setting it manually. It would not go through. This is seriously a issue, and caused abuncha issues until I realized what was wrong.
|
I added a issue for this: #685 |
|
LGTM. Can you add a test to https://github.com/npm/node-semver/blob/main/test/classes/semver.js, please? |
I didn't even notice the tests. Yes I will go create a test now, thank you for letting me know. :) |
|
Also what does LGTM stand for? |
Added test for format validation, between each supported build type.
I went ahead and added the test, can you please make sure they are up to your standards :) If i need to add comments, etc I'll gladly do it. |
|
Thanks for the test.
You should execute Personally, I think the formatted version should include build metadata because build is part of the semver spec. However, this would mean that the failing functions need to be adjusted so tests pass again. This is not my call and someone of the maintainers needs to way in how to proceed. |
| t.equal(semVer.format(), expected, `Format of ${version} should be ${expected}`); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
needs some spaces between { } and no semicolons:
test('format method produces correct version string', t => {
const formatTestCases = [
{ version: '1.2.3', options: {}, expected: '1.2.3' },
{ version: '1.2.3-alpha.1', options: {}, expected: '1.2.3-alpha.1' },
{ version: '1.2.3+build.11', options: {}, expected: '1.2.3+build.11' },
{ version: '1.2.3-alpha.1+build.11', options: {}, expected: '1.2.3-alpha.1+build.11' },
]
t.plan(formatTestCases.length)
formatTestCases.forEach(({ version, options, expected }) => {
const s = new SemVer(version, options)
t.equal(s.format(), expected, `Format of ${version} is ${s.format()} but should be ${expected}`)
})
})
|
thank you whoever let me see the results of the action <3 I'll go ahead and make the changes here shortly. Having a serious hard time getting |
|
don't spend more time on this until someone else has confirmed that this is the way to go |
|
Thanks @Bioblaze for your contribution and a BIG thanks to @mbtools for your help in this PR. We will need to take a very close look at how tools like npm use this module to be sure we aren't going to break everything if we do this. Unfortunately sometimes even though the code is doing something incorrect, all the tooling around it is built off the assumption that the current behavior is going to be consistent. We have recently added build info in other places, but because the build is NOT part of the version that's published we'll need to do this research. |
kk I've done changes for most of the tests. I'll go over a few more bits over the weekend and it should be good. I'll wait before I push XD I don't wanna break npm @.@ else my life is screwed lol |
|
As feared, this change would break npm. Unfortunately build is one of those historically confusing areas, in that it is (correctly) not considered part of the actual version for installation purposes. A good rundown of this can be found in this comment ~/D/n/s/semver $ npm pkg get version
"1.0.1-pre.0+alpha"
~/D/n/s/semver $ npm publish --dry-run
npm WARN publish npm auto-corrected some errors in your package.json when publishing. Please run "npm pkg fix" to address these errors.
npm WARN publish errors corrected:
npm WARN publish "version" was cleaned and set to "1.0.1-pre.0"
npm notice
npm notice 📦 semver@1.0.1-pre.0
npm notice === Tarball Contents ===
npm notice 384B package.json
npm notice 731B test.js
npm notice === Tarball Details ===
npm notice name: semver
npm notice version: 1.0.1-pre.0
npm notice filename: semver-1.0.1-pre.0.tgz
npm notice package size: 674 B
npm notice unpacked size: 1.1 kB
npm notice shasum: b0486e0410e96a17c7c56539cd86ae4ee5365b36
npm notice integrity: sha512-p3u2SjGSgvA6r[...]etGQgLGc46jbA==
npm notice total files: 2
npm notice
npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run)
+ semver@1.0.1-pre.0
~/D/n/s/semver $ node /Users/wraithgar/Development/npm/cli/branches/gar_semver-build publish --dry-run
npm notice
npm notice 📦 semver@1.0.1-pre.0+alpha
npm notice === Tarball Contents ===
npm notice 384B package.json
npm notice 731B test.js
npm notice === Tarball Details ===
npm notice name: semver
npm notice version: 1.0.1-pre.0+alpha
npm notice filename: semver-1.0.1-pre.0+alpha.tgz
npm notice package size: 674 B
npm notice unpacked size: 1.1 kB
npm notice shasum: b0486e0410e96a17c7c56539cd86ae4ee5365b36
npm notice integrity: sha512-p3u2SjGSgvA6r[...]etGQgLGc46jbA==
npm notice total files: 2
npm notice
npm notice Publishing to https://registry.npmjs.org/ with tag latest and default access (dry-run)
+ semver@1.0.1-pre.0+alphaIf build is something that is part of your module that matters, other solutions are going to have to be used. At the top of the list should be an awareness of what the contract is from this module, and how build fits into it. |
when you preformed format(), it was missing the this.build to be referenced. so if anyone was using this.build and setting it manually. It would not go through. This is seriously a issue, and caused abuncha issues until I realized what was wrong.
just adding this.build to the format() function