sarcasticadmin commented 3 months ago

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

[x] It's submitted to the main branch.
[x] 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).
[x] 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:

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
...
> 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

```bash
$ 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 commented 3 months ago

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 commented 3 months ago

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 :laughing:

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).

Right on, Ill give that a go and report back

sarcasticadmin commented 2 months ago

@mturoci

https://github.com/h2oai/wave/pull/2300/commits/7a4be6ef32135b8df9912b137dd4baef50c541ec 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 commented 2 months ago

Thanks @sarcasticadmin! Seems like we will need to wait for https://github.com/npm/cli/issues/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 commented 2 months ago

Thanks @sarcasticadmin! Seems like we will need to wait for https://github.com/npm/cli/issues/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 commented 2 months ago

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

👍

h2oai / wave

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

vite v4.5.3 building for production...