Finish the match key rename in the 51Did constants and make the linter runnable - #188
Merged
Merged
Conversation
The earlier rename left two static constants on FodId still carrying the old hash name, HASH_OFFSET and HASH_LENGTH, even though the comments on those two constants already described a match key. HEADER_LENGTH and PAYLOAD_LENGTH were plain numbers repeating the same values, so the relationship between them was not visible in the code. MATCH_KEY_OFFSET and MATCH_KEY_LENGTH are now the real constants, with the same values and the same comments. HASH_OFFSET and HASH_LENGTH stay as deprecated aliases pointing at the new names, worded the same way as the deprecated hash getter that already sits alongside matchKey, so code outside this repository keeps working while it moves over. HEADER_LENGTH and PAYLOAD_LENGTH are now worked out from the new names instead of repeating the numbers, and their own names do not change. Every use inside the repository now reads the new names, being the client, the example, the test helper and the tests. Nothing to do with SHA-256 hashing itself was renamed. The TypeScript definitions in fiftyone.pipeline.did/types/fodId.d.ts were regenerated from the JavaScript so the two agree. Verified by running the 51Did jest suite, which gave 139 passed and 2 skipped, and by running the 51Did example, which prints the same match key as before.
The repository lints with ESLint, and the nightly build runs npm run lint from the repository root, but the root package.json that carries the lint tooling was written out by ci/setup-environment.ps1 on every CI run and was never in the repository. Anyone with a clone had nothing to install and nothing to run, so the errors below went unnoticed for a long time even though the build printed them. The root package.json is now checked in, holding the same scripts and the same tooling versions the CI script used to generate, with the tooling moved into devDependencies to match how every package in this repository declares its own build tools. ESLint stays pinned at 8.57.0 and the plugins keep the caret ranges that CI has been proving for months. The package is marked private because it is only a harness and is never published. The generation step in ci/setup-environment.ps1 is gone, because leaving it would silently overwrite the checked-in file, and the notes it carried about the jest version and the quoting of the test patterns are kept as comments so that knowledge is not lost. Two changes to .eslintrc then clear thirteen of the fifteen errors. The first is ecmaVersion, which was 2020. The source has used class static fields since before this change and the tests use numeric separators, both of which arrived after 2020, so ESLint could not parse fiftyone.pipeline.did fodId.js or its test file at all and skipped every other rule in them. Raising it to 2022 parses both. The second is the jest globals. The globals list named test, jest and expect by hand but not describe, beforeAll or afterEach, so twelve uses of those three read as undefined variables. The three hand-listed names are replaced by the jest environment, applied through an overrides block so it reaches only the test files, the test helpers and setup.js rather than the whole repository. That leaves two real faults in the source, both now fixed rather than silenced. A resource key was read with bracket notation on a line whose neighbour already used a dot, and a trailing comment in the 51Did test file kept the padding from an earlier alignment that the match key rename had already broken. npm run lint now reports no errors, where it reported fifteen before. The 105 remaining warnings are the repository's existing JSDoc house style. The count rose from 71 only because the two files that could not be parsed now can be, and every one of the extra warnings comes from those two files. The 51Did suite still gives 139 passed and 2 skipped, and the whole repository suite run from the new root harness gives 269 passed across 23 suites.
jwrosewell
changed the title
The name member of IdType documented its return as a bare string type with no description at all, whilst fromFlags four lines above it says the number it returns is the IdType value. The gap stayed invisible until the parse error in this package was fixed, because the linter could not read the file to report it, and it reaches customers, as tsc copies the comment straight through into the published fiftyone.pipeline.did/types/idType.d.ts. The comment now states what a valid input is and what comes back. NAMES holds four entries, so name returns undefined for anything outside 0 to 3, and saying the input is 0 to 3 as fromFlags returns makes the declared string return true for every valid call. The declared type is deliberately left as string and not widened to allow undefined. The package is published on npm at 4.5.35, so widening it would stop TypeScript callers compiling against a version they already use. No behaviour changes, and no fallback or throw was added. The published type definition carries the same comment and is updated to match. Running the type generation from the root harness rewrites definition files across every package, because the harness resolves a different TypeScript patch version from the one that produced the committed files, so that churn was reverted and only idType.d.ts is committed here. The generation was run to confirm the hand edited definition is byte for byte what tsc produces from the new source comment. The linter goes from 105 warnings to 104 and no longer reports a missing returns description anywhere. The remaining 104 are left alone on purpose, because they are JSDoc formatting and documentation rules that catch no defect. The 51Did suite still gives 139 passed and 2 skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request does three things. It finishes the match key rename in the
51Did constants, it makes the linter runnable from a clone and clears every
error the linter reports, and it fixes the one documentation gap among the
warnings that reaches customers.
Part one, the match key rename
The problem
The rename of the 51Did hash to the match key
(#186) was only half
done.
FodIdgained amatchKeygetter with a deprecatedhashalias keptbeside it, but two of the static constants in
fiftyone.pipeline.did/fodId.jsstill carried the old name even though the comments written above those two
constants already said "match key".
A reader coming to this file has to work out that a constant called
HASH_OFFSETis where the match key starts, and thatHEADER_LENGTHandPAYLOAD_LENGTHare just the same two numbers written out again rather thanvalues worked out from them.
What changed
MATCH_KEY_OFFSETandMATCH_KEY_LENGTHare now the real constants. Thevalues (5 and 32) and the comments above them are unchanged.
HASH_OFFSETandHASH_LENGTHremain as deprecated aliases pointing atthe new names.
HEADER_LENGTHis nowFodId.MATCH_KEY_OFFSETandPAYLOAD_LENGTHis nowFodId.MATCH_KEY_OFFSET + FodId.MATCH_KEY_LENGTH, so the numbers are nolonger repeated. Neither of those two names changes, because neither is
about the match key.
fiftyone.pipeline.did/types/fodId.d.tswereregenerated with
tscfrom the sametsconfig.jsonthe build uses, so thedefinitions and the JavaScript agree, aliases and deprecation notes
included.
fodId.js(3),didClient.js(1),examples/fodIdExample.js(2),tests/envelope.js(3) andtests/fodId.test.js(16).the match key of a Probabilistic or HashedEmail identifier is a SHA-256
stays exactly as it was.
Two lines that the longer names pushed past 80 columns were wrapped, one in
the example and one in the constants test.
Why the aliases are kept
The package is published to npm as
fiftyone.pipeline.did,at 4.5.35 at the time of writing, and these two constants are part of its
public surface, so callers outside this repository may already read
FodId.HASH_OFFSETandFodId.HASH_LENGTHto build or slice a payload.Removing the old names in this change would break that code with no warning.
The aliases hold the same values, so old and new callers see the same numbers,
and the
@deprecatednote tells anyone still on the old names to move across.The wording of the note copies the deprecated
hashgetter that already sitsnext to
matchKeyin the same file, so the whole rename reads the same waywherever a reader meets it.
Part two, the linter
The problem
The repository has an ESLint setup and the build has been running it all
along, but nobody could run it from a clone, so the errors it prints have gone
unread.
.eslintrcsits at the repository root and covers every package. The linttooling it needs was declared only inside a PowerShell here string in
ci/setup-environment.ps1, which wrote a rootpackage.jsonfrom scratch onevery CI run. Nothing of that file was in the repository, so a clone had no
npm installto run and nonpm run lintto run either, and the only way tolint was to install ESLint out of tree and point it at the config by hand.
The build does run
npm run lint, throughcommon-ci
node/build-project.ps1, and it has been printing errors for a long time. Inthe nightly run on
mainof 2 September 2026(run 33583980519,
job Build and Test - 0 - Ubuntu_Node_24) the Build Project step printed this.
The step then moved straight on to Run Unit Tests and the job reported
success, so nothing was stopping those 15 errors accumulating.
The 15 errors were these.
A correction to what this pull request said earlier
An earlier version of this description said ESLint was not declared as a
dependency anywhere in the repository, and listed eight errors including five
reports of a missing
jsdoc/newline-after-descriptionrule. Both statementswere wrong and are corrected here.
ESLint was declared, in
ci/setup-environment.ps1, and the accurate statementis that the declaration was not in a file a clone could install from. The five
missing rule reports were an artefact of the out of tree install, which pulled
a current
eslint-plugin-jsdocin which that rule no longer exists. Theversion CI pins, 38.1.6, still defines it, and it is a warning there rather
than an error. The
no-multi-spaceserror was real but was hidden onmain,because the file it sits in could not be parsed at all, so no other rule in
that file ever ran.
What changed
The root
package.jsonis now in the repository. It carries the sameunit-test,integration-test,lintandtscscripts the CI script usedto generate, and the same tooling versions, so nothing about how the build
behaves changes. The tooling moved into
devDependencies, which is how everypackage in this repository already declares jest and TypeScript. ESLint stays
pinned exactly at 8.57.0 and the plugins keep the caret ranges CI has been
proving on every run, which matters because a caret on
eslint-plugin-jsdoc: ^38.1.6is what keeps the major version that removednewline-after-descriptionout. The package is markedprivatebecause it isa harness for running jest, tsc and ESLint against the local source and is
never published.
ci/setup-environment.ps1no longer writes that file. Leaving thegeneration in place would silently overwrite the checked in copy on every CI
run and let the two drift apart, which is the fault being fixed. The script
now just calls
./node/setup-environment.ps1, which runsnpm install. Thenotes the here string carried, about why jest has to be 28 or later and why
the test patterns use escaped double quotes rather than single quotes on
Windows, are kept as comments in the script so that knowledge survives.
.eslintrcecmaVersiongoes from 2020 to 2022. Class static fields areES2022 and numeric separators such as
0b0000_0101are ES2021, and the sourcehas used both since before this change, so ESLint could not parse
fiftyone.pipeline.did/fodId.jsorfiftyone.pipeline.did/tests/fodId.test.jsat all. Everything in those two files went unchecked. Raising the setting
parses both.
.eslintrcgains the jest environment. Theglobalslist namedtest,jestandexpectby hand and stopped there, sodescribe,beforeAllandafterEachread as undefined variables in the test files. The three handlisted names are replaced by
"env": { "jest": true }, which declares thewhole set the runner provides. It is applied through an
overridesblockmatching
**/tests/**/*.js,**/*.test.jsandsetup.js, so the jest namesreach the test files, the test helpers and the root setup file and no further.
Two faults in the source are fixed. In
fiftyone.pipeline.cloudrequestengine/tests/integration.test.jsa resource keywas read as
process.env['_51DEGREES_RESOURCE_KEY']on a line whose own nextline already used
process.env.RESOURCE_KEY, so it now readsprocess.env._51DEGREES_RESOURCE_KEYand matches its neighbour. Infiftyone.pipeline.did/tests/fodId.test.jsa trailing comment kept the paddingfrom an alignment that the match key rename in
#186 had already broken,
so the padding is gone.
junit.xmlandtest-results/are added to.gitignore, because now thatnpm run unit-testcan be run from a clone it drops a reporter file at theroot.
No rule was turned off and no
eslint-disablecomment was added anywhere.Which fixes are config changes, and why
Two of the four are config changes rather than source changes, and in both
cases the config was wrong and the source was right.
ecmaVersiondescribes the language the source is written in. The source isvalid modern JavaScript that runs on every Node version this repository
supports, being 20, 22 and 24. Editing the source to suit a 2020 setting would
mean giving up class static fields and numeric separators for no reason, so
the setting is what had to move.
describe,beforeAllandafterEachare real globals that jest injects intoa test file. They are not undefined, they were undeclared. The config was
already trying to say this by hand and had simply missed most of the names, so
naming the environment is both the supported way to say it and the way that
cannot go stale again.
How it was verified
Values read back from the built class, which confirms the aliases and the
worked out lengths still give the numbers the format needs.
The 51Did suite, run from
fiftyone.pipeline.didon Node v24.13.0.The same command on
maingives the same totals, being 139 passed and 2skipped in 3 passed suites with 1 suite skipped, so this change neither adds
nor loses a test and the skips are not related to it.
The whole repository suite, run from the newly checked in root harness the way
the build runs it.
The linter, run the normal way with no out of tree install. This is a clean
clone of this branch, then
npm install, thennpm run lint, which is thewhole point of part two.
So the count goes from 15 errors to 0, and
npm run lintnow exits 0. The104 is after part three below removes one warning. Part two alone leaves 105.
The warnings rise from 71 to 105 and every one of the 34 extra warnings comes
from the two files that could not be parsed before, being
fodId.jswith 28and
fodId.test.jswith 6. No other file gained a warning. What they are, andwhat was done about them, is part three.
The generated TypeScript definitions still build, checked with
npm run tscfrom the root, which exits 0. That command rewrites the
.d.tsfiles for everypackage, and only the ones this change is about,
fiftyone.pipeline.did/types/fodId.d.tsandfiftyone.pipeline.did/types/idType.d.ts, are committed here. The churn itproduced in the other packages was reverted, because it comes from the root
harness resolving a different TypeScript patch version from the one that
generated the committed files and has nothing to do with this change. That
drift is worth a look on its own.
Part three, the one warning worth fixing
Clearing the parse errors in part two made 105 warnings readable for the first
time. They were then read, one by one, rather than counted. All 105 come from
the
jsdocplugin and not one reports a defect in behaviour, but one of themis a real documentation gap that customers can see.
The problem
In
fiftyone.pipeline.did/idType.js, thenamemember declared its returnwith no description at all, whilst
fromFlagsfour lines above it says whatits return means.
This is not only an internal comment.
tsccopies it straight through intothe published
fiftyone.pipeline.did/types/idType.d.ts, so a customer readingthe type definitions in their editor sees a documented
fromFlagsnext to anundocumented
name.What changed
The comment now says what a valid input is and what comes back, in both the
source and the definition file, which carry identical text.
No code changed, no fallback was added and nothing throws.
Why the declared return type was not widened
NAMESholds exactly four entries, soname(type)hands backundefinedforany value outside 0 to 3, and a reader could argue the honest declaration is
string | undefined. It is deliberately left asstring.The package is published on npm as
fiftyone.pipeline.didat 4.5.35, and
name(type: number): stringis in the type definitionscustomers already compile against. Widening it to include
undefinedwouldbreak every TypeScript caller that assigns the result to a
string, at compiletime, in a patch release, over a comment. Naming the precondition on the
parameter instead makes the existing
stringdeclaration true, because forevery input the documentation now admits, a string is exactly what comes back.
If the type should be widened, that belongs in a release where a breaking
change is expected and announced, not here.
Why the other 104 are left alone
They were grouped by rule and read.
jsdoc/newline-after-descriptionjsdoc/require-jsdocjsdoc/require-description@paramor@returnsalready describes the member in full, wanting separate prose above the tags as well.jsdoc/check-types@returns {function}preferring{Function}, in a test helper comment that is compiled into nothing.None of the 104 reports a defect in behaviour. Two of them sit on the
deprecation blocks written for the aliases in part one, which are written the
same way as the deprecated
hashgetter they copy, so fixing only those twowould make them the only JSDoc blocks in the repository written differently
from every other.
eslint --fixis not the answer either. Asked with--fix-dry-run, whichwrites nothing, it clears 100 of the 105 and then creates 165 new ones, taking
the total to 170, because the autofix for
jsdoc/require-jsdocinserts emptystubs like this one, which then trip
require-param-type,require-param-descriptionandrequire-descriptionin turn.For completeness,
--fixhere is safe even though it is unhelpful. Every fileit would rewrite was tokenized before and after with the
espreeparser ESLintitself uses, and all 11 token streams are identical, so every change it
proposes sits inside a comment and none can alter behaviour.
The two noisy rules are worth revisiting separately.
jsdoc/newline-after-descriptionandjsdoc/require-jsdocproduce 99 of the104, against a house style the repository does not follow and against test and
example helpers where a doc comment adds nothing. Turning them down, or at
least not applying
require-jsdoctotestsandexamples, would leave ashort list somebody will actually read. A warning list nobody reads is how 15
real errors sat unnoticed in the build log for months, which is the fault part
two fixes. That change is not made here because it is a judgement about house
style rather than part of this rename.
How part three was verified
The linter, from the repository root.
Down from 105, and
jsdoc/require-returns-descriptionno longer appearsanywhere in the output. The remaining rules are
jsdoc/newline-after-description67,jsdoc/require-jsdoc32,jsdoc/require-description4 andjsdoc/check-types1, which is the same setas before minus the one fixed here.
The 51Did suite is unchanged.
npm run tscexits 0. The definition file was hand edited rather than takenfrom a full generation run, to avoid the cross package churn described above,
and the generation was then run once anyway to check the hand edited file is
byte for byte what
tscproduces from the new source comment. It is. The churnthat run created in the other packages was reverted, so the only definition
file committed in this part is
fiftyone.pipeline.did/types/idType.d.ts.Produced with AI assistance under James Rosewell's direction and needs human review.