Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

fix: Reverse the direction of the semver check #16

Merged
merged 3 commits into from
Mar 5, 2021
Merged

fix: Reverse the direction of the semver check #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
347eb4b
Reverse the direction of the semver check
dyladan Mar 3, 2021
8dcbbbe
chore: fix test
dyladan Mar 4, 2021
2b60af9
test: rewrite semver test with more clarity
dyladan Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/internal/global-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ export function registerGlobal<Type extends keyof OTelGlobalAPI>(
export function getGlobal<Type extends keyof OTelGlobalAPI>(
type: Type
): OTelGlobalAPI[Type] | undefined {
const version = _global[GLOBAL_OPENTELEMETRY_API_KEY]?.version;
if (!version || !isCompatible(version)) {
const globalVersion = _global[GLOBAL_OPENTELEMETRY_API_KEY]?.version;
if (!globalVersion || !isCompatible(globalVersion)) {
return;
}
return _global[GLOBAL_OPENTELEMETRY_API_KEY]?.[type];
Expand Down
85 changes: 49 additions & 36 deletions src/internal/semver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,27 @@ const re = /^(\d+)\.(\d+)\.(\d+)(?:-(.*))?$/;
*
* The returned function has the following semantics:
* - Exact match is always compatible
* - Major versions must always match
* - The minor version of the API module requesting access to the global API must be greater or equal to the minor version of this API
* - Major versions must match exactly
* - 1.x package cannot use global 2.x package
* - 2.x package cannot use global 1.x package
* - The minor version of the API module requesting access to the global API must be less than or equal to the minor version of this API
* - 1.3 package may use 1.4 global because the later global contains all functions 1.3 expects
* - 1.4 package may NOT use 1.3 global because it may try to call functions which don't exist on 1.3
* - If the major version is 0, the minor version is treated as the major and the patch is treated as the minor
* - Patch and build tag differences are not considered at this time
*
* @param ownVersion version which should be checked against
*/
export function _makeCompatibilityCheck(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes me wonder if we couldn't just vendor semver package ? Seems error prone to re-implement the logic ourselves. That would weight for browser but i'm just throwing the idea out there

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semver satisfies is more strict than we are. This could be a good thing though. If global is 1.3.3, we will be able to call it from 1.3.2 and 1.3.4. With the semver package we would only be able to call it from 1.3.2. Semver will never say an earlier version of a package satisfies. This would require end-user applications to always have the latest patch versions. Honestly may not be such a bad thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can pimp a version like 1.3.3 to 1.3.x then semver.satisfies() will be fine with 1.3.0 and 1.3.5.
semver offers a lot APIs to parse/modify versions. It's also possible to e.g. include pre-releases.

I think if we need something in version checking which semver can't do it's most likely problematic.

Main question is do we want one more dependency in API. Don't think there is any real world node application out there which hasn't semver already in it's node_modules tree somewhere.
Can't tell for browser.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without semver:

test/index-webpack.js    3.49 MiB  test/index-webpack

With semver:

test/index-webpack.js    3.66 MiB  test/index-webpack

Looks like a 0.17 MiB difference if we add the dependency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think there is any real world node application out there which hasn't semver already in it's node_modules tree somewhere

In my experience this is about right

Can't tell for browser.

I suspect it's much lower since semver is mostly used for dependency management and the browser doesn't often have reason for that after compilation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can pimp a version like 1.3.3 to 1.3.x then semver.satisfies() will be fine with 1.3.0 and 1.3.5.
semver offers a lot APIs to parse/modify versions. It's also possible to e.g. include pre-releases.

Yes these things are for sure possible. We need to decide what behavior we want in this regard either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to decide what behavior we want in this regard either way.

If we don't get concensus right now, i think we could still refactor this later as its purely internal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I revamped the test suite to be much more comprehensive and much more clear 2b60af9. Makes me more comfortable leaving the semver package out of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its purely internal.

It's not purely internal. If we adopt semver, we will get stricter checks for prerelease versions. If we release our version, then 1.0.0, 1.0.0-alpha.1, and 1.0.0-alpha.2 will all be compatible with each other in both directions, which means we will be unable to make any changes once we release RC.

ownVersion: string
): (version: string) => boolean {
): (globalVersion: string) => boolean {
const acceptedVersions = new Set<string>([ownVersion]);
const rejectedVersions = new Set<string>();

const myVersionMatch = ownVersion.match(re);
if (!myVersionMatch) {
throw new Error('Cannot parse own version');
// we cannot guarantee compatibility so we always return noop
return () => false;
}

const ownVersionParsed = {
Expand All @@ -46,66 +52,73 @@ export function _makeCompatibilityCheck(
patch: +myVersionMatch[3],
};

return function isCompatible(version: string): boolean {
if (acceptedVersions.has(version)) {
function _reject(v: string) {
rejectedVersions.add(v);
return false;
}

function _accept(v: string) {
acceptedVersions.add(v);
return true;
}

return function isCompatible(globalVersion: string): boolean {
if (acceptedVersions.has(globalVersion)) {
return true;
}

if (rejectedVersions.has(version)) {
if (rejectedVersions.has(globalVersion)) {
return false;
}

const m = version.match(re);
if (!m) {
const globalVersionMatch = globalVersion.match(re);
if (!globalVersionMatch) {
// cannot parse other version
rejectedVersions.add(version);
return false;
// we cannot guarantee compatibility so we always noop
return _reject(globalVersion);
}

const otherVersionParsed = {
major: +m[1],
minor: +m[2],
patch: +m[3],
const globalVersionParsed = {
major: +globalVersionMatch[1],
minor: +globalVersionMatch[2],
patch: +globalVersionMatch[3],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semver also has "-+", considering we are about to release an "-RC1" this starts to get complicated...
By the time we add the extra checks how much more of that 0.17Mib would get consumed, while I'll big on minification, I'm also big on don't re-invent the wheel (unless absolutely necessary for perf or size). And as most CDN payloads are also GZip'd how much does that 0.17 shrink too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just noticed the tests below using "alpha".. so ignore the above about pre-release and build#

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone note the time @MSNev advocated for an increased bundle size 🤣

};

// major versions must match
if (ownVersionParsed.major !== otherVersionParsed.major) {
rejectedVersions.add(version);
return false;
if (ownVersionParsed.major !== globalVersionParsed.major) {
return _reject(globalVersion);
}

// if major version is 0, minor is treated like major and patch is treated like minor
if (ownVersionParsed.major === 0) {
if (ownVersionParsed.minor !== otherVersionParsed.minor) {
rejectedVersions.add(version);
return false;
}

if (ownVersionParsed.patch < otherVersionParsed.patch) {
rejectedVersions.add(version);
return false;
if (
ownVersionParsed.minor === globalVersionParsed.minor &&
ownVersionParsed.patch <= globalVersionParsed.patch
) {
return _accept(globalVersion);
}

acceptedVersions.add(version);
return true;
return _reject(globalVersion);
}

if (ownVersionParsed.minor < otherVersionParsed.minor) {
rejectedVersions.add(version);
return false;
if (ownVersionParsed.minor <= globalVersionParsed.minor) {
return _accept(globalVersion);
}

acceptedVersions.add(version);
return true;
return _reject(globalVersion);
};
}

/**
* Test an API version to see if it is compatible with this API.
*
* - Exact match is always compatible
* - Major versions must always match
* - The minor version of the API module requesting access to the global API must be greater or equal to the minor version of this API
* - Major versions must match exactly
* - 1.x package cannot use global 2.x package
* - 2.x package cannot use global 1.x package
* - The minor version of the API module requesting access to the global API must be less than or equal to the minor version of this API
* - 1.3 package may use 1.4 global because the later global contains all functions 1.3 expects
* - 1.4 package may NOT use 1.3 global because it may try to call functions which don't exist on 1.3
* - If the major version is 0, the minor version is treated as the major and the patch is treated as the minor
* - Patch and build tag differences are not considered at this time
*
* @param version version of the API requesting an instance of the global API
Expand Down
160 changes: 96 additions & 64 deletions test/internal/semver.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,81 +21,113 @@ import {
} from '../../src/internal/semver';
import { VERSION } from '../../src/version';

describe('Version Compatibility', () => {
describe('semver', () => {
it('should be compatible if versions are equal', () => {
assert.ok(isCompatible(VERSION));
});

describe('throws if own version cannot be parsed', () => {
assert.throws(() => {
_makeCompatibilityCheck('this is not semver');
});
it('returns false if own version cannot be parsed', () => {
const check = _makeCompatibilityCheck('this is not semver');
assert.ok(!check('1.0.0'));
});

describe('incompatible if other version cannot be parsed', () => {
it('incompatible if other version cannot be parsed', () => {
const check = _makeCompatibilityCheck('0.1.2');
assert.ok(!check('this is not semver'));
});

describe('>= 1.x', () => {
it('should be compatible if major and minor versions are equal', () => {
const check = _makeCompatibilityCheck('1.2.3');
assert.ok(check('1.2.2'));
assert.ok(check('1.2.2-alpha'));
assert.ok(check('1.2.4'));
assert.ok(check('1.2.4-alpha'));
});

it('should be compatible if major versions are equal and minor version is lesser', () => {
const check = _makeCompatibilityCheck('1.2.3');
assert.ok(check('1.1.2'));
assert.ok(check('1.1.2-alpha'));
assert.ok(check('1.1.4'));
assert.ok(check('1.1.4-alpha'));
});

it('should be incompatible if major versions do not match', () => {
const check = _makeCompatibilityCheck('3.3.3');
assert.ok(!check('0.3.3'));
assert.ok(!check('0.3.3'));
});

it('should be incompatible if major versions match but other minor version is greater than our minor version', () => {
const check = _makeCompatibilityCheck('1.2.3');
assert.ok(!check('1.3.3-alpha'));
assert.ok(!check('1.3.3'));
});
describe('>= 1.0.0', () => {
const globalVersion = '5.5.5';
const vers: [string, boolean][] = [
// same major/minor run should be compatible
['5.5.5', true],
['5.5.6', true],
['5.5.4', true],

// if our version has a minor version increase, we may try to call methods which don't exist on the global
['5.6.5', false],
['5.6.6', false],
['5.6.4', false],

// if the global version is ahead of us by a minor revision, it has at least the methods we know about
['5.4.5', true],
['5.4.6', true],
['5.4.4', true],

// major version mismatch is always incompatible
['6.5.5', false],
['6.5.6', false],
['6.5.4', false],
['6.6.5', false],
['6.6.6', false],
['6.6.4', false],
['6.4.5', false],
['6.4.6', false],
['6.4.4', false],
['4.5.5', false],
['4.5.6', false],
['4.5.4', false],
['4.6.5', false],
['4.6.6', false],
['4.6.4', false],
['4.4.5', false],
['4.4.6', false],
['4.4.4', false],
];

test(globalVersion, vers);
});

describe('0.x', () => {
it('should be compatible if minor and patch versions are equal', () => {
const check = _makeCompatibilityCheck('0.1.2');
assert.ok(check('0.1.2'));
assert.ok(check('0.1.2-alpha'));
});

it('should be compatible if minor versions are equal and patch version is lesser', () => {
const check = _makeCompatibilityCheck('0.1.2');
assert.ok(check('0.1.1'));
assert.ok(check('0.1.1-alpha'));
});

it('should be incompatible if minor versions do not match', () => {
const check = _makeCompatibilityCheck('0.3.3');
assert.ok(!check('0.2.3'));
assert.ok(!check('0.4.3'));
});

it('should be incompatible if minor versions do not match', () => {
const check = _makeCompatibilityCheck('0.3.3');
assert.ok(!check('0.2.3'));
assert.ok(!check('0.4.3'));
});

it('should be incompatible if minor versions match but other patch version is greater than our patch version', () => {
const check = _makeCompatibilityCheck('0.3.3');
assert.ok(!check('0.3.4-alpha'));
assert.ok(!check('0.3.4'));
});
describe('< 1.0.0', () => {
const globalVersion = '0.5.5';
const vers: [string, boolean][] = [
// same minor/patch should be compatible
['0.5.5', true],

// if our version has a patch version increase, we may try to call methods which don't exist on the global
['0.5.6', false],

// if the global version is ahead of us by a patch revision, it has at least the methods we know about
['0.5.4', true],

// minor version mismatch is always incompatible
['0.6.5', false],
['0.6.6', false],
['0.6.4', false],
['0.4.5', false],
['0.4.6', false],
['0.4.4', false],

// major version mismatch is always incompatible
['1.5.5', false],
['1.5.6', false],
['1.5.4', false],
['1.6.5', false],
['1.6.6', false],
['1.6.4', false],
['1.4.5', false],
['1.4.6', false],
['1.4.4', false],
];

test(globalVersion, vers);
});
vmarchaud marked this conversation as resolved.
Show resolved Hide resolved
});

function test(globalVersion: string, vers: [string, boolean][]) {
describe(`global version is ${globalVersion}`, () => {
for (const [version, compatible] of vers) {
const alphaVersion = `${version}-alpha.1`;
it(`API version ${version} ${
compatible ? 'should' : 'should not'
} be able to access global`, () => {
const check = _makeCompatibilityCheck(version);
assert.strictEqual(check(globalVersion), compatible);

// alpha tag should have no effect different than the regular version
const alphaCheck = _makeCompatibilityCheck(alphaVersion);
assert.strictEqual(alphaCheck(globalVersion), compatible);
});
}
});
}