fix: Reverse the direction of the semver check #16
Conversation
dyladan
requested review from
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## main #16 +/- ##
==========================================
- Coverage 94.65% 94.64% -0.02%
==========================================
Files 39 39
Lines 505 504 -1
Branches 80 81 +1
==========================================
- Hits 478 477 -1
Misses 27 27
Continue to review full report at Codecov.
|
|
Bumping minor when adding a new API method is what we will do. Before This Fixapp API version behind dependency API version
app API version ahead of dependency API version
After This Fixapp API version behind dependency API version
app API version ahead of dependency API version
|
| * - 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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const globalVersionParsed = { | ||
| major: +globalVersionMatch[1], | ||
| minor: +globalVersionMatch[2], | ||
| patch: +globalVersionMatch[3], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok, just noticed the tests below using "alpha".. so ignore the above about pre-release and build#
There was a problem hiding this comment.
Someone note the time @MSNev advocated for an increased bundle size 🤣
The semver check is backwards. It has not yet caused a bug because we haven't released any other 0.18.x
In the current 0.18 semver check if the user registers a global with version 0.18.0, then API module 0.18.1 attempts to access it, it will succeed. 0.18.1 may attempt to call a method which doesn't exist on 0.18.0. It should be the other way around. 0.18.0 should be able to access the 0.18.1 global because that global has at least the methods 0.18.0 expects.
I caught this while testing the semver check with fake versioning in my sample project