Repository navigation
Take OWID as a submodule, as the other languages do - #107
Merged
Merged
Conversation
fiftyone.pipeline.did requires ./owid-js/v1.js from the owid-js submodule, pinned at the commit the GitHub dependency named (6c5ff4e7), and its files carry v1.js and the Apache 2.0 LICENSE beside it. The GitHub dependency and bundleDependencies go, and with them the step in ci/build-package.ps1 that installed a bundled dependency before packing. Jest and ESLint leave the submodule's own tests and code alone.
# Conflicts: # ci/build-package.ps1
The cloud 51Did workflow checked out no submodules, as the package used to take OWID from a GitHub reference at install. fiftyone.pipeline.did now requires ./owid-js/v1.js from the owid-js submodule, so the workflow fetches that one submodule before installing.
tests/packaging.test.js reads remote_package.json, which only the repository has, so in an installed copy three of its tests fail.
It reads remote_package.json, which Publish moves onto package.json, so it can only run in the repository.
… packed The nightly submodule update runs submodule update --remote for every submodule, so owid-js follows main of SWAN-community/owid-js through a pull request into rc, and the readme no longer names a commit.
jwrosewell
marked this pull request as ready for review
November 14, 2026 12:31
This was referenced Nov 14, 2026
Open
Draft
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
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.
fiftyone.pipeline.did took the OWID library as the npm dependency
"owid": "github:SWAN-community/owid-js#6c5ff4e7", bundled into its tarball, where every other 51Degrees language takes OWID as a git submodule. This takes owid-js as the submodulepipeline/fiftyone.pipeline.did/owid-js, pinned at the same commit, and packs only the library'sv1.jsand itsLICENSEfrom it.Closes #78.
What the issue says, and what was found
The issue says npm downloads owid-js from GitHub "whenever the package is installed". The red run below shows that is not quite what happens. The published package bundles OWID under
node_modules/owid, so an install with GitHub blocked succeeds. What the GitHub reference does cause, today:npm installat the root resolves the GitHub reference, which needs git and GitHub, and is the only dependency here that does.rclists 63 files, 16 of them undernode_modules/owid, being its GitHub workflow, eight test files, two test data files,setupJest.js,README.md,package.json,LICENSEandv1.js. The package needs onlyv1.js.npm lsfails in a project that installs the package. In the red run, with GitHub blocked,npm installof the packed package succeeded, andnpm ls --allthen failed withnpm error code ELSPROBLEMSandowid@0.1.0 invalid: "github:SWAN-community/owid-js#6c5ff4e7" from node_modules/fiftyone.pipeline.did, because npm checks the bundled copy against the GitHub reference independencies.What the specification says
npm's package.json documentation (npm 11), under files:
and under bundleDependencies:
So a bundled package still has its version in
dependencies, here the GitHub reference, which is whatnpm lschecks the bundled copy against.Like
.gitignore, a pattern with no slash matches by file name at every depth. npm 11.6.2's packlist hands thefilesentries to ignore-walk 8.0.0, which reads each with minimatch andmatchBase: true, and minimatch appliesmatchBaseonly to a pattern of one path segment. So*.jsalso matchesowid-js/owid.test.jsonceowid-js/v1.jshas opened that folder, whilst/*.jsmatches only files at the top of the package. The green run of 00:20 (run 43532860022), on an earlier form of this change with*.js, packedowid-js/setupJest.jsand everyowid-js/*.test.js.owid-js is under the Apache License 2.0, whose section 4, in the submodule's
LICENSEat the pinned commit, says:owid-js has no
NOTICEfile, so theLICENSEis the one file that has to travel withv1.js.What changed
Files are linked at the head, 58fa1096.
.gitmodulesaddspipeline/fiftyone.pipeline.did/owid-jswith the absolute URLhttps://github.com/SWAN-community/owid-js.git, as .NET, Java, Python, PHP and Rust have it, pinned at 6c5ff4e7, the commit the GitHub reference named.fodId.jsrequires./owid-js/v1.jsin place ofowid, andtests/fodId.test.jsandtests/reader.test.jsrequire and mock that path.package.jsonandremote_package.jsondrop theowiddependency andbundleDependencies, and gain afileslist. It keeps what the package packed before, being/*.js,internal/,types/,examples/,tests/andtsconfig.json, and addsowid-js/v1.jsandowid-js/LICENSE, with!tests/packaging.test.jsleaving out the one test that readsremote_package.json, which only the repository has. The package's own Jest settings leave out/owid-js/.ci/build-package.ps1no longer installs a bundled dependency before packing, as no package has one. The rootunit-testscript leaves outowid-js/, and.eslintignoreleaves out**/owid-js/, so neither runs or lints the submodule's own tests and code.mainthrough a pull request intorc, and to fetch the submodule, and AGENTS.md has an OWID section saying whyfilesanchors/*.jsand leaves out the packaging test, and how the submodule moves..github/workflows/cloud-51did.yml, which runs the live 51Did tests on pull requests intorcsince #100, checked out no submodules, because the install used to fetch OWID from GitHub. It now fetches the owid-js submodule alone before installing.The test,
tests/packaging.test.js, checks that neither manifest names a dependency on GitHub or bundles one, that both listowid-js/v1.jsandowid-js/LICENSEand no other file of the submodule, that both manifests pack the same files, thatnpm pack --dry-run --jsonlists exactly those two files underowid-js/and every.jsfile at the top of the package and not the packaging test itself, and that the package'sFodIdpasses every OWID parse status through.Runs
Each throwaway run uses a workflow with two parts. The test jobs install the workspace, lint, and run the 51Did tests as the unit tests do, on Ubuntu with Node.js 22 and 24 and on Windows with Node.js 24. The pack job packs the package with
remote_package.jsonin place as Publish does, compares the file list with the published 4.5.102, blocks GitHub in/etc/hostsand checks that it is blocked, installs the tarball into an empty project, runsnpm ls --all, lists the OWID files installed, and runs the 51Did tests on a copy of the installed package.rcac7b6386, which addstests/packaging.test.jsalone. In every test job six of its seven tests failed ("Tests: 6 failed, 346 passed, 352 total"), the one that passed being that both manifests pack the same files, which neither lists. The pack job packed 63 files, bundling 16 undernode_modules/owid, installed with GitHub blocked, and failed atnpm ls --allas quoted above.rcmerge, e685ff9c and the throwaway workflow, and nothing else. Every test job ran "Tests: 352 passed, 352 total". The pack job packed 49 files where 4.5.102 has 59. Against 4.5.102 the 16 files undernode_modules/owidare gone,owid-js/LICENSEandowid-js/v1.jsare added, and so are four test files written since 4.5.102 (base64.test.js,liveCloudKeys.js,packaging.test.js,tierEntitlement.integration.test.js). With GitHub blocked the install added 1 package,npm ls --allpassed withfiftyone.pipeline.did@4.5.0and nothing under it, and the 51Did tests on the installed copy ran "Tests: 209 passed, 209 total".tests/packaging.test.json the installed copy, whereremote_package.jsondoes not exist, which is why the packaging test is now left out of the tarball. The workflow then dropped that file from the step, which is the only difference in 1fd7bf7.rca9a3bd2 was merged in at fb2b267. Its one conflict was inci/build-package.ps1, which #37 rewrote to set each package's version and requirements. The merge keeps Give each package its own version and release only what changed #37's version of that file less its step that installed a bundled dependency, which this draft removes.tests/packaging.test.js, and the pack job packed 50 files.npm ls --allshowing onlyfiftyone.pipeline.did@4.5.0, and the 51Did tests on the installed copy ran "Tests: 209 passed, 209 total".tmp/pr-matrix-107c, success. Configure, all six Build and Test jobs (Ubuntu x64 and ARM and Windows, Node.js 22 and 24) and Complete are green. All 7Setting 'PullRequestSha'lines name the head 58fa1096, and Configure printed "Left out 2 macOS entries".tmp/pr-matrix-107b, success. Configure, all six Build and Test jobs (Ubuntu x64 and ARM and Windows, Node.js 22 and 24) and Complete are green. All 7Setting 'PullRequestSha'lines name fd8fa115, and Configure printed "Left out 2 macOS entries".macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
What remains
git submodule update --remotefor every submodule (steps/update-sub-modules.ps1line 9 in common-ci at 1c5c0e0b), so a new commit on owid-jsmainreaches this package through the nightly's pull request intorc, as OWID does in the other languages. The GitHub reference this replaces moved only by hand.filesalso names*.jswithout the slash. It packs nothing extra today, as its submodule has no.jsfile, and its body says so.