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

sarcasticadmin · 2024-03-27T02:25:27Z

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

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:

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
Is the package-lock.json reasonable?
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
...
> [email protected] 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
...

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 · 2024-03-28T08:30:32Z

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 [email protected].
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 · 2024-03-28T16:33:54Z

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 [email protected].
Check if hashes were added (e.g. via git diff).

Right on, Ill give that a go and report back

sarcasticadmin · 2024-04-06T18:47:58Z

@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 · 2024-04-12T10:47:59Z

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 · 2024-04-12T16:13:33Z

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 · 2024-04-15T05:46:46Z

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

👍

sarcasticadmin requested review from lo5 and mturoci as code owners March 27, 2024 02:25

sarcasticadmin force-pushed the rh/1711501146bump2 branch from 0df999a to 7f5da38 Compare March 27, 2024 04:04

sarcasticadmin force-pushed the rh/1711501146bump2 branch from 7f5da38 to 6ab9b12 Compare April 5, 2024 17:56

second pass at packages

7a4be6e

sarcasticadmin force-pushed the rh/1711501146bump2 branch from 6ab9b12 to 7a4be6e Compare April 6, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sarcasticadmin commented Mar 27, 2024 •

edited

mturoci commented Mar 28, 2024

sarcasticadmin commented Mar 28, 2024

sarcasticadmin commented Apr 6, 2024

mturoci commented Apr 12, 2024

sarcasticadmin commented Apr 12, 2024 •

edited

mturoci commented Apr 15, 2024

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

Are you sure you want to change the base?

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

Conversation

sarcasticadmin commented Mar 27, 2024 • edited

System information

mturoci commented Mar 28, 2024

sarcasticadmin commented Mar 28, 2024

sarcasticadmin commented Apr 6, 2024

mturoci commented Apr 12, 2024

sarcasticadmin commented Apr 12, 2024 • edited

mturoci commented Apr 15, 2024

sarcasticadmin commented Mar 27, 2024 •

edited

sarcasticadmin commented Apr 12, 2024 •

edited