fix: ui dependencies in package-lock.json missing resolved/integrity fields

[sarcasticadmin]

The PR fulfills these requirements: (check all the apply)

• It's submitted to the main branch.
• When resolving a specific issue, it's referenced in the PR's title (e.g. feat: Add a button #xxx, where "xxx" is the issue number).
• When resolving a specific issue, the PR description includes Closes #xxx, where "xxx" is the issue number.
• If changes were made to ui folder, unit tests (make test) still pass.
• New/updated tests are included

Closes #2299

The following PR attempts to maintain the versions of node packages thats closest to the existing packages-lock.json. I was unable to find a way to simple populate the missing resolved/integrity fields in the original packages-lock.json

The process was as follows:

$ rm -rf node_modules package-lock.json
# commit to be stricter for some packages (will explain why below)
$ npm install
$ npm install # This seemed to be required to add in eslint-plugin-wave entry
$ nix-build # see default.nix below

The result builds successfully. However, I have a couple of outstanding items I wanted to raise with the maintainers:

1. npm test - I see a lot of failures but I suspect this could be due to being on node 18 since I see a ton of failures off main
2. Is the package-lock.json reasonable?
3. The reason I add additional strictness for some of the packages in packages.json was that I was getting the following error if I just ran rm -rf node_modules package-lock.json && npm install. Whats odd is that react was still on ~17.0.2:

$ nix-build
...
> wave-ui@0.26.0 build
> npx vite build

vite v4.5.3 building for production...
=============

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.

You may find that it works just fine, or you may not.

SUPPORTED TYPESCRIPT VERSIONS: >=3.3.1 <4.5.0

YOUR TYPESCRIPT VERSION: 4.9.5

Please only submit bug reports when using the officially supported version.

=============
Warning: React version specified in eslint-plugin-react-settings must be a valid semver version, or "detect"; got “latest”
Warning: React version specified in eslint-plugin-react-settings must be a valid semver version, or "detect"; got “latest”
Warning: React version specified in eslint-plugin-react-settings must be a valid semver version, or "detect"; got “latest”
Warning: React version specified in eslint-plugin-react-settings must be a valid semver version, or "detect"; got “latest”
Warning: React version specified in eslint-plugin-react-settings must be a valid semver version, or "detect"; got “latest”
✓ 2 modules transformed.
✓ built in 4.50s
[vite-plugin-eslint]
/build/ui/src/index.tsx
  36:1  error  ReactDOM.render is deprecated since React 18.0.0, use createRoot instead, see https://reactjs.org/link/switch-to-createroot  react/no-deprecated

✖ 1 problem (1 error, 0 warnings)

file: /build/ui/src/index.tsx
error during build:
RollupError:
/build/ui/src/index.tsx
  36:1  error  ReactDOM.render is deprecated since React 18.0.0, use createRoot instead, see https://reactjs.org/link/switch-to-createroot  react/no-deprecated

✖ 1 problem (1 error, 0 warnings)

    at error (file:///build/ui/node_modules/rollup/dist/es/shared/node-entry.js:2287:30)
    at Object.error (file:///build/ui/node_modules/rollup/dist/es/shared/node-entry.js:25351:20)
    at Object.error (file:///build/ui/node_modules/rollup/dist/es/shared/node-entry.js:24472:42)
    at Object.transform (/build/ui/node_modules/vite-plugin-eslint/dist/index.js:1:2469)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async transform (file:///build/ui/node_modules/rollup/dist/es/shared/node-entry.js:24449:16)
    at async ModuleLoader.addModuleSource (file:///build/ui/node_modules/rollup/dist/es/shared/node-entry.js:24649:30)

ERROR: `npm build` failed
...

4. Unsure why running a second npm install added the eslint-plugin-wave entry. But maybe this is because it comes from wave-ui itself?

System information

$ node --version
v18.19.1
$ npm --version
10.2.4
$ nixos-version
23.11.20240117.8bf65f1 (Tapir)

default.nix placed in the ui subdir:

{ pkgs ? import
    (fetchTarball {
      url = "https://github.com/NixOS/nixpkgs/archive/d3f8923899a36a1985b58804de2b0cb0862015cd.tar.gz";
      sha256 = "sha256:0m2jf3chas1dls1gk6q2dg8q9ciq8mmbxbbmpfi5hjj8wd09a5kx";
    })
    { }
}:
let
  #version = "1.1.1";
  #src = pkgs.fetchFromGitHub {
  #  owner = "h2oai";
  #  repo = "wave";
  #  rev = "v${version}";
  #  sha256 = "sha256-fINuoJx7dPN613wLLzcC2aar5vz6L6qzAWm/bWgj9bo=";
  #};
  src = ./.;
in
pkgs.buildNpmPackage {
  #inherit version src;
  inherit src;
  name = "waved-ui";

  #sourceRoot = "${src.name}/ui";

  npmDepsHash = "sha256-nBd1qMxu57ven0hP/Qxh7uRUBMJOnXOp7RhtWJ3zotA=";

  nodejs = pkgs.nodejs_18;

  npmFlags = [ "--ignore-scripts" ];

  installPhase = ''
    runHook preInstall

    mkdir $out
    cp -r build $out/www
    
    runHook postInstall
  '';
}

[mturoci]

Thanks for the PR @sarcasticadmin!

npm test - I see a lot of failures but I suspect this could be due to being on node 18 since I see a ton of failures off main

I suppose you didn't run npm ci which respects package-lock.json, but used npm install which doesn't install the locked versions. I tried with Node 18 and tests pass.

Is the package-lock.json reasonable?

Hm... not really. I would expect to only see integrity hashes added where missing. This, however, also updates versions.

The reason I add additional strictness for some of the packages in packages.json was that I was getting the following error if I just ran rm -rf node_modules package-lock.json && npm install

rm -rf node_modules package-lock.json && npm install seems to not be the way to go here. Let's try the following instead:

• Find all the packages in package-lock that do not have integrity hash specified, e.g. react. (tip: use jq).
• Run npm i <lib>@<exact_version>, so in this case npm i react@17.0.2.
• Check if hashes were added (e.g. via git diff).

I only tested with react, and it seems to work. Needs to be verified for the rest of the packages though.

[sarcasticadmin]

I suppose you didn't run npm ci which respects package-lock.json, but used npm install which doesn't install the locked versions. I tried with Node 18 and tests pass.

Nope I didnt run it that way, will retry with npm ci. Thanks for the tip

Hm... not really. I would expect to only see integrity hashes added where missing. This, however, also updates versions.

I would agree. You'd think npm would make this easier 😆

Let's try the following instead:

Find all the packages in package-lock that do not have integrity hash specified, e.g. react. (tip: use jq).
Run npm i @<exact_version>, so in this case npm i react@17.0.2.
Check if hashes were added (e.g. via git diff).

Right on, Ill give that a go and report back

[sarcasticadmin]

@mturoci

7a4be6e was a result of the following commands. The oneliner excessive but I think it proves out the point adding the exact versions via npm. This was ran against all packages in packages.json since only doing the subset of missing SHAs seemed like it would lead to more version drift if we didnt explicitly call all packages:

$ cp package-lock.json orig-package-lock.json
$ rm -rf node_modules package-lock.json
$ cat orig-package-lock.json | \
jq -r '.packages | to_entries[] | if .value.dev == true then .value.dev = "--save-dev" end | [.key,.value.version,.value.dev // ""] | @tsv' | \
sed 's/^node_modules\///' | \
grep -v 'node_modules' | \
tail -n +3 | tr -s '[:blank:]' '@' | \
rev  |  sed 's/@/ /' | rev | \
xargs -I {} npm i {}

• jq - For packages, if a package is for dev the set --save-dev instead so we can include it later in npm i. Convert output to tsv to split later
• sed- strip the node_modules from the name if it at the beginning
• grep - Remove any packages that are sub dependencies of the actual package
• tail - Display all lines from 3 onward. This removes the wave ui lib itself and the wave eslinter for it from the list.
• tr - Replace all blank lines with an @ to mimic the version declaration in npm
• rev | sed | rev - Replace trailing @ with a space in-between version and --save-dev
• xargs - run npm install for each package

Some additional comments:

• "dev": true disappeared from a lot of packages. Its unclear to me why this would have happened (assuming all dev deps were defined correctly)? I was expecting --save-dev should have prevented this
• Im not sure why --save-dev was appended to all the dev=true packages in packages.json. I was going to omit all the package.json changes anyway once we really only cared for the SHAs but curious why this happened.
• There are still discrepancies in package versions (seems better than the original commits though). It seems that the order in which the npm package was installed leads to versions still not being exact for dependencies. Im also omitting the sub dependency versions of packages (if Im understanding that correctly). Initially, I suspect this is due to the graph thats being built for only packages with the resolved key missing. But this diff was running npm i through all packages so that it settles all versions or at least gets closer to whats on main.

[mturoci]

Thanks @sarcasticadmin! Seems like we will need to wait for npm/cli#4460 to provide official way to fix this.

In the meantime, I am wondering whether there is a way to disable the nix check.

[sarcasticadmin]

Thanks @sarcasticadmin! Seems like we will need to wait for npm/cli#4460 to provide official way to fix this.

I doubt npm will get to this anytime soon, this has been open since 2022.

I'll take another stab at this by calculating the integrity and including resolved and integrity per package in the package-lock.json. That should get us to where we need to be, its just a little more brute force. Does that sound reasonable?

In the meantime, I am wondering whether there is a way to disable the nix check.

I opted to use the published release for the ui components: https://github.com/sandra-radio/ebb/blob/c3b3644e7d3141cda7e93cea1aec362a25792d15/nix/pkgs/waved.nix#L18-L30

[mturoci]

I'll take another stab at this by calculating the integrity and including resolved and integrity per package in the package-lock.json. That should get us to where we need to be, its just a little more brute force. Does that sound reasonable?

Pretty hacky, but I guess there is no other way to do this properly. Sounds good to me!

I opted to use the published release for the ui components

👍