feat: per-version uuid validation#542
Conversation
broofa
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Suggesting some minor changes if we decide to take this, however we should have @ctavan and others weigh in before you do more here.
My main concern is that validating uuid versions is already trivially done with version(). E.g. version(uuid) === 1. This expands the API surface significantly, but I'm not really sure it's adding much value.. 😦
|
hey @broofa, I like your suggestions. The main reason for this is because of a project specifically checking for v4 UUIDs, we were using the uuidv4 which deprecated and switched to internally redirected to uuid's validate, which wasn't fit for purpose in our project. I think there is still value for people being able to call for per-version validation. I'm going to revise my branch. |
adds specific validation for all uuid versions should be 100% backward compatible
|
@broofa what do you think to this revision? I've used an internal call to As it looks like you like monolithic commits on here, I amended and force-pushed on my fork. |
| return typeof uuid === 'string' && REGEX.test(uuid); | ||
| } | ||
|
|
||
| export default validate; |
There was a problem hiding this comment.
I think it makes sense to use only named exports in this module.
There was a problem hiding this comment.
not sure what you mean?
Most other things seem to be done as default exports.
There was a problem hiding this comment.
They don't have named exports. Validate is not that special to use different export syntax than other methods in this module.
There was a problem hiding this comment.
Ahhh, I understand what you mean now. Good point.
I agree that'd be more consistent with the rest of the source. I'll revise my branch.
consistent with the rest of the library's style, in line with suggestion by @TrySound
|
IMO so much fragmented source code is awful (hard to maintain). And this is exactly the reason I personally always avoid default exports. |
@TrySound I'm not a fan, but I appreciate the importance of consistent style rather than one specific style, so I'm happy to do it to stay in-line with the rest of the library's source. |
Does this add enough value for enough users to warrant inclusion in this module, though? Validation functions are trivially easy to implement, as demonstrated by the implementations in this PR. It may be simpler/better to direct users to roll their own so they get the exact behavior they need (e.g. including The cost of including these is mostly the API and documentation bloat. While it may not seem that big a deal, this affects a lot (100K's or M's?) of users, 99% (???) of whom won't need these methods.
Does file fragmentation affect dead code removal ("tree-shaking")? The current function-per-file schema is an artifact of the CommonJS deep-include approach in version 3 of this module, but that's been deprecated so there's no compelling reason for it. Assuming bundlers are smart about removing dead code within a module we can structure the files in whatever way makes the most sense. For example, the v1-v5 methods are large and distinct enough they should probably be separate (I wouldn't expect much disagreement on that), but the other methods could go in a 'util.js` file. |
|
This feature request has been raised a couple of times in the past, see #526 (comment) and #498 (comment) and my suggestion has always been to simply make use of the existing API and write out the one-liner validate(uuid) && version(uuid) === uuidVersion;that this pull request implements. Now this PR adds 239 lines of code and I'm with @broofa in that I think this is really a lot of overhead to maintain in the future for something that can already be achieved by combining two existing API calls in one single line of code. So unless we find really compelling reasons of why having to learn about another 4 methods of the API surface is significantly better for end users than to just combine two existing ones my tendency is to not accept this pull request. I would, however, be more than happy to accept a pull request that improves the documentation and provides the above mentioned one-liner as a copy & pastable example. |
|
@ctavan 160 of those lines are in the documentation and 47 are in testing, but I see your point. |
This is precisely the point 😉 . It's documentation and tests that are tedious to maintain on the long run.
Where have you been looking for it (to then be disappointed not to find version-specific validation)? |
Hahah, I suppose by the What about a new |
|
I agree with you in principle, but the question is if people will find it if it appears in another section? I personally wouldn't mind just adding another short paragraph and another code block after the |
|
Makes sense 👍 |
|
Haven't runmd yet, but what do you think to this? |
|
For my personal taste this becomes to verbose. I'd restrict it to one simple example: import { version as uuidVersion, validate as uuidValidate } from 'uuid';
function uuidValidateV4(uuid) {
return validate(uuid) && version(uuid) === 4;
}
const v1Uuid = 'd9428888-122b-11e1-b85c-61cd3cbb3210';
const v4Uuid = '109156be-c4fb-41ea-b1b4-efe1671c5836';
console.log(uuidValidateV4(v4Uuid)); // RESULT
console.log(uuidValidateV4(v1Uuid)); // RESULTAnd I'd also not put it under a new section but just as a second example in the validate section. |
|
Closed in favour of #543 |

adds specific validation for all uuid versions
should be 100% backward compatible