flatironinstitute / mcmc-monitor

Monitor MCMC runs in the browser
Other
35 stars 0 forks source link

version of node #11

Closed magland closed 1 year ago

magland commented 1 year ago

I have reason to think that mcmc-monitor has an error with v14 of nodejs, which is the default version on the FI cluster (module load node-js), whereas it works fine with v16, installed via conda. The error has to do with the Buffer type. I think we should put in the docs that this requires node v >=16.

jsoules commented 1 year ago

This sounds good. We should also probably put in a check on launching the service, so that the user gets a specific error message for the known issue.

jsoules commented 1 year ago

So I've found two ways to do this.

To do the first one, per stack overflow, we would want to add an engines block to the project's package.json and also add an .npmrc file with engine-strict=true as a key. The advantage of this is that it provides an error at install time, though the disadvantage is that it's somewhat complicated and relies on technically-deprecated functionality (specifically the engines block in package.json; the .npmrc file is then required to make that enforced rather than just a warning.) Also, ultimately, this can be bypassed by a persistent-enough user who wants to install a lot of packages manually.

The advantage of the check-node-version package is that it's easier and requires less fussy config, but the downside is that it is yet another dependency, and as a runtime check, it wouldn't provide the user with any warning until they actually try to run the service, which is a frustrating UX.

I think ultimately that we should probably do both, so that users get a warning at installation time (when it's first actionable) and also at runtime if they somehow manage to bypass that challenge (or if they fail to turn on an environment module, etc).

magland commented 1 year ago

If the engines block in package.json is deprecated then I don't think we should use it.

Note that in our case, the installation and run steps are bundled together since the run command is "npx ..."

jsoules commented 1 year ago

Sorry, it isn't the engines block that's deprecated, it's the engineStrict flag in package.json which is no longer recognized. engines still works, but without the .npmrc setting to make it strict, it will be a warning only.

npx is convenient but I'm not sure we can count on it always--it seems like without doing some global install, npx may fail to cache properly, so we might need to update the instructions for people who don't want to/can't do global node package installs. (I'm still looking for confirmation on whether this is a known issue or some weird thing only we are encountering.)

jsoules commented 1 year ago

Closed via PR #29.