Skip to content

Take OWID as a submodule, as the other languages do - #107

Merged
jwrosewell merged 7 commits into
rcfrom
feature/owid-submodule
Nov 14, 2026
Merged

jwrosewell merged 7 commits into
rcfrom
feature/owid-submodule

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

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 submodule pipeline/fiftyone.pipeline.did/owid-js, pinned at the same commit, and packs only the library's v1.js and its LICENSE from 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:

  1. Working in this repository clones owid-js through npm. npm install at the root resolves the GitHub reference, which needs git and GitHub, and is the only dependency here that does.
  2. The tarball carries the whole of owid-js. The red run's pack of rc lists 63 files, 16 of them under node_modules/owid, being its GitHub workflow, eight test files, two test data files, setupJest.js, README.md, package.json, LICENSE and v1.js. The package needs only v1.js.
  3. npm ls fails in a project that installs the package. In the red run, with GitHub blocked, npm install of the packed package succeeded, and npm ls --all then failed with npm error code ELSPROBLEMS and owid@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 in dependencies.

What the specification says

npm's package.json documentation (npm 11), under files:

File patterns follow a similar syntax to .gitignore, but reversed:

and under bundleDependencies:

This defines an array of package names that will be bundled when publishing the package.

Note that the package names do not include any versions, as that information is specified in dependencies.

So a bundled package still has its version in dependencies, here the GitHub reference, which is what npm ls checks 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 the files entries to ignore-walk 8.0.0, which reads each with minimatch and matchBase: true, and minimatch applies matchBase only to a pattern of one path segment. So *.js also matches owid-js/owid.test.js once owid-js/v1.js has opened that folder, whilst /*.js matches 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, packed owid-js/setupJest.js and every owid-js/*.test.js.

owid-js is under the Apache License 2.0, whose section 4, in the submodule's LICENSE at the pinned commit, says:

(a) You must give any other recipients of the Work or Derivative Works a copy of this License; and

owid-js has no NOTICE file, so the LICENSE is the one file that has to travel with v1.js.

What changed

Files are linked at the head, 58fa1096.

  1. The submodule. .gitmodules adds pipeline/fiftyone.pipeline.did/owid-js with the absolute URL https://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.
  2. The package uses it. fodId.js requires ./owid-js/v1.js in place of owid, and tests/fodId.test.js and tests/reader.test.js require and mock that path.
  3. The manifests. package.json and remote_package.json drop the owid dependency and bundleDependencies, and gain a files list. It keeps what the package packed before, being /*.js, internal/, types/, examples/, tests/ and tsconfig.json, and adds owid-js/v1.js and owid-js/LICENSE, with !tests/packaging.test.js leaving out the one test that reads remote_package.json, which only the repository has. The package's own Jest settings leave out /owid-js/.
  4. The build. ci/build-package.ps1 no longer installs a bundled dependency before packing, as no package has one. The root unit-test script leaves out owid-js/, and .eslintignore leaves out **/owid-js/, so neither runs or lints the submodule's own tests and code.
  5. The notes. The 51Did readme says how the library is taken, that the nightly submodule update moves it to the newest commit on owid-js main through a pull request into rc, and to fetch the submodule, and AGENTS.md has an OWID section saying why files anchors /*.js and leaves out the packaging test, and how the submodule moves.
  6. The cloud 51Did workflow. .github/workflows/cloud-51did.yml, which runs the live 51Did tests on pull requests into rc since #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 list owid-js/v1.js and owid-js/LICENSE and no other file of the submodule, that both manifests pack the same files, that npm pack --dry-run --json lists exactly those two files under owid-js/ and every .js file at the top of the package and not the packaging test itself, and that the package's FodId passes 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.json in place as Publish does, compares the file list with the published 4.5.102, blocks GitHub in /etc/hosts and checks that it is blocked, installs the tarball into an empty project, runs npm ls --all, lists the OWID files installed, and runs the 51Did tests on a copy of the installed package.

  • Red, run 43560445019 on 3d8570c, failure. Its parent is the test commit de236c9 on rc ac7b6386, which adds tests/packaging.test.js alone. 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 under node_modules/owid, installed with GitHub blocked, and failed at npm ls --all as quoted above.
  • Green, run 43561084776 on 1fd7bf7, success. Its tree is the head before the rc merge, 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 under node_modules/owid are gone, owid-js/LICENSE and owid-js/v1.js are 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 --all passed with fiftyone.pipeline.did@4.5.0 and nothing under it, and the 51Did tests on the installed copy ran "Tests: 209 passed, 209 total".
  • The first green attempt on e685ff9, run 43560456147, packed and installed the same way, then failed only because its last step ran tests/packaging.test.js on the installed copy, where remote_package.json does 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.
  • rc a9a3bd2 was merged in at fb2b267. Its one conflict was in ci/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.
  • The Cloud 51Did check failed on the earlier head e685ff9, run 43561115753, with "Cannot find module './owid-js/v1.js' from 'fodId.js'", because its workflow checked out no submodules. With fd8fa115 fetching the submodule there, it passed on fd8fa11, run 43582176311, and on the head 58fa109, run 43588006130, as did the CI script tests on Ubuntu and Windows, run 43588006046.
  • The review of fd8fa11 found that the nightly does move owid-js, and that the packaging test was packed and fails in an installed copy. The test commit 3cc89e8 adds the check that the test is not packed, 54d6d0f leaves it out of both manifests, and 58fa1096 corrects the readme and AGENTS.md.
  • Red, run 43587966486 on fe9701e, failure. Its parent is the test commit 3cc89e8. In each test job only the new check failed ("Tests: 1 failed, 352 passed, 353 total"), npm's list holding tests/packaging.test.js, and the pack job packed 50 files.
  • Green, run 43587973289 on e04eccf, success. Its parent is the head 58fa1096. Every test job ran "Tests: 353 passed, 353 total", the pack job packed 49 files, without the packaging test, installed them with GitHub blocked with npm ls --all showing only fiftyone.pipeline.did@4.5.0, and the 51Did tests on the installed copy ran "Tests: 209 passed, 209 total".
  • Full build, run 43588006138 from 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 7 Setting 'PullRequestSha' lines name the head 58fa1096, and Configure printed "Left out 2 macOS entries".
  • An earlier full build on fd8fa11, run 43582200826 from 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 7 Setting '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

  • owid-js now moves with the nightly submodule update, which runs git submodule update --remote for every submodule (steps/update-sub-modules.ps1 line 9 in common-ci at 1c5c0e0b), so a new commit on owid-js main reaches this package through the nightly's pull request into rc, as OWID does in the other languages. The GitHub reference this replaces moved only by hand.
  • #98 adds a package whose files also names *.js without the slash. It packs nothing extra today, as its submodule has no .js file, and its body says so.

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.
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
jwrosewell marked this pull request as ready for review November 14, 2026 12:31
@jwrosewell
jwrosewell merged commit 6d5d4fe into rc Nov 14, 2026
5 checks passed
@jwrosewell
jwrosewell deleted the feature/owid-submodule branch November 14, 2026 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant