ENH: Update JS lockfile, add CI job to build JS package

[connortann]

Supports #3100

Changes:

• Adds a fix for ERR_OSSL_EVP_UNSUPPORTED error (see below).
  • Underlying issue tracked in BUG: outdated openssl provider in javascript package #3102
• Updates the Javascript dependencies (only non-breaking updates) with npm update.
• Adds a CI job to ensure that npm run build passes

Nb. This PR does not change the existing bundle.js, which is used by the main python package. I think we should park this until we have built some confidence in the javascript package, ideally updating the major versions of dependencies and having a test suite in place.

We should give this PR a careful review: ideally it would be great to have a JS developer validate that nothing is broken, given that we do not have JS unit tests specified.

[codecov]

Codecov Report

Merging #2974 (830cac4) into master (a0219d0) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2974      +/-   ##
==========================================
- Coverage   55.35%   55.33%   -0.02%     
==========================================
  Files          90       90              
  Lines       12881    12881              
==========================================
- Hits         7130     7128       -2     
- Misses       5751     5753       +2     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[connortann]

@SachinVarghese , as the original author of this JavaScript package (as far as I can tell), would you be happy to advise on how we can validate if this update will cause any issues?

[remborg]

npm run dev fails with ERR_OSSL_EVP_UNSUPPORTED because the version of node we're using is more recent (>=17) than when the project was initially developed. This error happens with npm run build too.

An option is to set the environment variable NODE_OPTIONS to --openssl-legacy-provider. This apparently supplies OpenSSL implementations of algorithms that have been deemed legacy (not used anymore, insecure, etc.).
So, on a linux machine, package.json file dev script would need to be updated from:
"dev": "webpack-dev-server --content-base build/ --open",
to
"dev": "export SET NODE_OPTIONS=--openssl-legacy-provider && webpack-dev-server --content-base build/ --open",

The best option would be to upgrade all the packages to the latest version instead, but then shap will have to be fully and manually tested...

[connortann]

That's really helpful @remborg , thanks! With the OpenSSL legacy variable set, npm run dev runs ok and ends with Compiled with warnings.

There are still a number of reported vulnerabilities when npm update is called. I tried npm audit fix, which recommended some updates which would be breaking changes:

• webpack-dev-server@4.15.1
• react@18.2.0
• d3@7.8.5

Do you have any advice on how best to test the javascript package? If we can put together a good test, we could potentially put together a CI job which runs when anything in the javascript directory is changed. This would be helpful when we get suggested updates from dependabot.

[remborg]

I believe that the best way would be to build the project in the pipeline and see if webpack is throwing errors, but this won't tell if it's functionally working.

I personally use Typescript to help catch incompatible packages early, but it's not a silver bullet.

Tech debt will have to be paid eventually, so I suggest updating all the dependencies and bumping shap's to the next major version (so go to v1.0.0 if that's an option). Unfortunately someone who knows the library well will have to test it manually...

[connortann]

I created an issue #3100 to define tests for the package, which is probably a pre-requisite for us evaluating dependency updates.

[connortann]

@remborg I've added a CI job to build the JS package, which now passes. It checks that webpack -p passes, which is better than nothing. However, it would be even better to check that the example plot can be created ok. Do you have any advice on how that could be implemented?

The dev command webpack-dev-server --content-base build/ --open generates an example page at localhost:8080, using the file javascript/build/index.html. Maybe we could check this renders ok on CI - is there a typical pattern for this kind of thing?

[remborg]

Great job Connor. To test that the react component is rendering properly, you can create a snapshot of it (an HTML copy of the rendered component) https://jestjs.io/docs/snapshot-testing

It is also possible to write visual regression tests using jest-image-snapshot (to compare what the component looks like. Comparing a stored picture with a new one after the new changes applied) but this has server dependencies that could be an issue (puppeteer, a headless browser, sometimes some graphic drivers too, etc.).

[connortann]

That's helpful, thanks. I note there is already a file javascript/test.js, which I'm guessing is a unit test. I tried running that with Jest, by running npm install --save-dev jest, and adding this to package.json:

"scripts": {
    "test": "jest",
}

However, npm test then fails with SyntaxError: Cannot use import statement outside a module.

[connortann]

@thatlittleboy I feel ready to submit this PR for reviewing now.

As per the description, I haven't updated the main bundle.js in the repo in this PR, as I think we should park that for when we have more comprehensive JS tests up and running.

For now, I think doing the minor version updates (which includes a fair few critical fixes) and building the package on CI is a good place to start.

[thatlittleboy]

@connortann thanks for taking this up and thanks @remborg for the advice/directions given thus far! Really appreciate it.

I'm not particularly adept at JS, much less so on JS packaging, so I don't feel comfortable doing the review for this PR.

That said, given that the dependencies are very old and in need of updating, I'm think I'm in favour of merging in this PR and fixing any bugs that come along as a result (if any).

[connortann]

@thatlittleboy I was talking offline to remborg who may be able to offer some more help with JS testing tomorrow, so let's hold on merging for a few days...

[remborg]

@connortann I've create a Pull Request with my changes here: #3106

[connortann]

@remborg thanks for your PR, I approved and merged your changes into this branch.

It looks like deleting test.js breaks the npm run build, as that file is included in the webpack bundle:

// webpack.config.js line 26
module.exports = [
  {
    entry: {
      bundle: "./index.jsx",
      test_bundle: "./test.js"
    },

Can you see value in keeping that file in the webpack bundle - could it perhaps be used to manually test the bundle.js that is created by webpack?

[remborg]

Sorry, I've missed this. We need the 'index.jsx' file to be part of the bundle.
I think we should bring back the 'test.js' file, merge it with the other deleted file 'random-explanation.js' and maybe rename it to 'test_bundle.js'?

This file simply calls a JS function and append the output in the page. I have replicated that in the new 'tests/render.spec.jsx' file with the snapshot.

[connortann]

Thanks @remborg , thanks for looking over this issue and giving your input. It was very helpful to have a sense-check from an experienced JS dev.

@thatlittleboy as you suggest I will go ahead and merge.