kurokida / jsQUEST

A Bayesian adaptive psychometric method for measuring thresholds in online experiments.
MIT License
17 stars 4 forks source link

Is the numeric.js updated? #4

Closed kurokida closed 3 years ago

kurokida commented 3 years ago

Hi @tpronk , thank you in advance.

I've added the version information to src/jsQUEST.js, and compiled it. This is the commit https://github.com/kurokida/jsQUEST/commit/b2735afceaea88218cb6cb4a10893faf0cd9ad4a

At this point, I noticed that the numeric.js was also updated. Does this mean a change of the numeric in your repository? Do I need to be concerned?

tpronk commented 3 years ago

I recompiled jsQUEST in my fork, and I get the same differences in output. I'm not sure why it happened, but if the bundle still acts the same I wouldn't worry about it. Packages like rollup do all kinds of magic with the code that I don't understand, but since it's a very established package, I tend to assume it's fine.

Your question does inspire me to bring up testing and debugging tools. I'll post it in a separate issue.

tpronk commented 3 years ago

I think there might have been an issue after all: if I reinstall the jsQUEST repo and run the test below, I got an error... node test/jsQUEST_test.js

Going a bit deeper I discovered that that package-lock.json file refers to an earlier commit of numeric that still was buggy.

I've prepared a PR that fixes this issue. It also features an extra bundle of jsQUEST in ES format. Actually, I discovered the bug while I was creating this extra bundle to upgrade the way we import jsQUEST in PsychoJS.

My apologies for the mistake.

kurokida commented 3 years ago

Thank you for making the pull request. I've merged it.

But I have a few concerns. The problem with package-lock.json may have been caused by my computer.

After merging the latest pull request from you, I tried to run npm install command. The following messages appeared.

npm install npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2. I'll try to do my best with it! npm WARN optional SKIPPING OPTIONAL DEPENDENCY: fsevents@2.3.2 (node_modules\fsevents): npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for fsevents@2.3.2: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

updated 2 packages and audited 23 packages in 0.918s

4 packages are looking for funding run npm fund for details

found 0 vulnerabilities

Then, a new package-lock.json file was created. Please refer to the previous and current files I attached to this post.

package-lock_Before_684afef931ca847e0559b449da17a881e2e7bc13.json.txt package-lock.json-current.txt

In fact, I pushed the package-lock.json file from my computer three days ago. This might cause a problem you faced.

tpronk commented 3 years ago

I'm not sure how the error slipped in, but I think it was already on my side as well. Do the tests and demos work well on your side?

About package-lock.json. Yes, I had to update my npm for some PsychoJS-related work. Maybe we should add to the docs which one is required, though probably people that would install node today, would also get a recent version of npm. May I know which operating system you use? Then I can give you a pointer.

kurokida commented 3 years ago

Thank you for your reply. Yes, both the tests and demos work well.

The operating system in my PC is Windows 10 Home 64 bit. The npm version is 6.4.13.

This means that I am not facing any problem, but if I upload my package-lock.json file to the repo and you fork/download it, the error happens on your computer.

Do we need to publish the package-lock.json file in our repository? I think this file will be created automatically on each person's computer when they run thenpm install command.

tpronk commented 3 years ago

package-lock.json is a bit more precise than package.json in specifying which versions of a dependency to install, but generally it's not that important. I tend commit package-lock.json out of habit. So sure, we could leave it out.

If you would like to keep it in and ensure you don't get any warnings, you'll need to update npm to v7.x. For example via their installer.

kurokida commented 3 years ago

Now I installed the latest version of node. The version of node and npm is 16.30 and 7.18.1, respectively. The problem with the package-lock.json seems to have been resolved. Let's keep the file in the repo. Thank you for your advise!