benjamin-heasly
commented
2 years ago Thanks @paulstey for kicking off the build, and @eldu for your feedback. I looked into the error it was putting up and I'll try to summarize what I saw / did.
The build was trying to run node app.js and failing on new import statements within app.js and other files. I tried adding babel to the build and running node app.js on the version compiled by babel, and this fixed the import error.
But even so, the same step later failed when jspsych 7 tried to access the window. I think this makes sense, if this app and jspsych are intended for browser use rather than via node directly. With this in mind I removed the node app.js step from the build. This allows the build to proceed to the Jest tests, which do run and pass.
I'm thinking the Jest tests are covering more than node app.js, and so maybe this is sufficient?
But I could be missing something and we really do want to run node app.js as part of the build. If so I could look into that further, and I might ask your advice on how to work around the window.
Thanks!
broarr
eldu
Greetings @mirestrepo, @eldu, @hollandjg! Could I ask for another review on the way to upgrading honeycomb with jspsych 7?
This PR is intended to have no functional changes, but makes a few upgrades that came out of my local testing with honeycomb and jspsych 7. I found that the jspsych 7 upgrades went beyond the honeycomb repo itself, and into this repo as well. Once I made these changes locally, I was able to run through the honeycomb demo task using jspsych 7.
As a summary:
package-lock.jsonandyarn.lock, adding some noisejspsych-reactwithjspsych7require()style to esimportstyle@babel/preset-envas a dev dependency so that tests still work withimportChanging from
require()toimportseemed invasive and I'm not sure what you think!My reasoning was:
require()didn't seem to work, the results were string names of obscure.cjsfiles, rather than the expected function or class symbols.require()ing es modules is not expected to work.import.