chore(deps): replace semver with compare-versions to reduce bundle size #4170
Conversation
|
Also any advice on the best ways to test this, I have sanity-checked the compare-versions methods and confirmed they behave the same way as semver. The unit tests pass (also helped catch a bug 🥳) anything else needed? |
Codecov Report
@@ Coverage Diff @@
## main #4170 +/- ##
==========================================
- Coverage 92.50% 90.52% -1.99%
==========================================
Files 275 159 -116
Lines 7369 3757 -3612
Branches 1540 835 -705
==========================================
- Hits 6817 3401 -3416
+ Misses 552 356 -196 |
| ): boolean { | ||
| if (typeof version === 'undefined') { | ||
| // If we don't have the version, accept the wildcard case only | ||
| return supportedVersions.includes('*'); | ||
| } | ||
|
|
||
| return supportedVersions.some(supportedVersion => { | ||
| return satisfies(version, supportedVersion, { includePrerelease }); | ||
| return supportedVersion === '*' || satisfies(version, supportedVersion); |
There was a problem hiding this comment.
Hmm, looks like this is not the same behavior anymore. In the gRPC instrumentation, we set the supported version to 1.*, but this now evaluates to false with any 1.x.x version (which is why the tests fail).
There was a problem hiding this comment.
this may affect lot's of third-party instrumentations and instrumentations over at https://github.com/open-telemetry/opentelemetry-js-contrib
There was a problem hiding this comment.
created: omichelsen/compare-versions#70
There was a problem hiding this comment.
Hey, currently looking at this! Thanks for creating that issue :)
I see a few community packages doing this too
This package also just specifies 1 as the version?
There was a problem hiding this comment.
I tested out adding a ^ in front of the version in the instrumentation-grpc package and that does seem to resolve this issue but I have listened to too many of Linus's rants to think shipping this satisfies behaviour change as it stands is a good idea 😆
There was a problem hiding this comment.
I have opened a PR to fix this on compare-versions omichelsen/compare-versions#71
I will see where we get too
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
This PR was closed because it has been stale for 14 days with no activity. |
Which problem is this PR solving?
Semver makes up 1/4 of the output of our bundled opentelemetry distribution whilst not providing much utility.
In aws-lambda environments, this contributes significantly to the coldstart (about 30ms)
Fixes # (issue)
#3537
Short description of the changes
replaces semver with compare-versions in
The API should be comparable and behaviour should be very similar.
With the changes in instrumentation, the prerelease flag is no longer needed because this is a default of the satisfies method is not broken by prerelease tags
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: