bjoluc / jspsych-builder

A CLI utility to easily develop and package jsPsych experiments
MIT License
46 stars 12 forks source link

Issue with file path #11

Closed jannisbosch closed 2 years ago

jannisbosch commented 2 years ago

Hi @bjoluc, sorry for bothering you again, but I'm working with the jspsych-builder a lot right now... I'm actually not a 100% sure if the issue lies with jspsych-builder or with me and my setup, but my knowledge is not deep enough to solve that before posting here... As to the issue: Since a couple of days (and I'm suspecting it's since the latest update) I can not get jspsych init to run anymore. The shell throws the following error:

File not found with singular glob: /C:/Users/username/AppData/Roaming/nvm/v16.13.1/node_modules/jspsych-builder//assets/template/package.tmpl.json 

which to me looks like an issue with the file path formatting. I first suspected the interaction between jspsych-builder and nvm for windows to cause the issue, so I uninstalled nvm as well as the jspsych-builder and reinstalled node and the builder. However, the error remains. My second pc (also windows) has the same issue. So I'm starting to think it might be a general issue. I did some digging into your code and suspect the issue to lie with the URL(import.meta.url).pathname function. Sorry in advance if the error is caused by my own setup, I tried all I could to make sure it's not ;)

Edit: I think I found out where the error lies. I'm not sure it's restricted to windows... In tasks.js:

const builderDir = new URL("..", import.meta.url).pathname;

resolves as /C:/Users/username/directory/ instead of C:/Users/username/directory

Haffi921 commented 2 years ago

I get the same issue:

File not found with singular glob: /C:/.../bin/node_modules/jspsych-builder//assets…

However, I feel like there's some bug with adding /C:/.../bin/node_modules/jspsych-builder/ and /assets paths together. Meaning it doesn't remove the slash between. Just an idea.

Haffi921 commented 2 years ago

Yeah I think I found the problem. In tasks.js this is the code. You create a new path/url from where jspsych-builder index script is imported from and go up one directory. Well that causes the path to have a trailing slash (e.g. .../jspsych-builder/).

const builderDir = new URL("..", import.meta.url).pathname;
const builderAssetsDir = builderDir + "/assets";
const builderNodeModulesDir = builderDir + "/node_modules";

Then you simply add the "/assets" and "/node_modules" to that path causing it to have two slashes between. I think using path.resolve fixes this without needing to calculate whether you need the slash in "/assets" or not. In fact it's better because no matter what, path.resolve will fix the path up. Would that be an option?

bjoluc commented 2 years ago

Thanks @jannisbosch and @Haffi921 for flagging this! I set up windows jobs on GH Actions now so I hope I'll spot problems like this before publishing next time – sorry! It seems that I introduced the problem when I replaced const builderDir = path.resolve(__dirname, ".."); with const builderDir = new URL("..", import.meta.url).pathname; while migrating to ES6. Unfortunately, the duplicate forward slash wasn't the root problem (https://github.com/bjoluc/jspsych-builder/runs/4507175563?check_suite_focus=true). Another "problem" is that glob explicitly requires forward slashes on windows, so most of the path methods might not work in this setting... I'm wondering how path.resolve(__dirname, ".."); previously did the job though, as – according to the docs – it returns a normalized path (including backslashes)...

Haffi921 commented 2 years ago

I have a fix that I can put as a pull request or just copy it. Will set up in a minute. Basically the fix is using the function url.fileURLToPath instead of the URL class. That ensures the path that comes out is valid on all platforms. Then you can use path.resolve.

Obviously, I don't know the whole scope of why you changed from path.resolve to the URL so you need to evaluate this yourself :)

Haffi921 commented 2 years ago

Oh sorry, didn't notice you actually put up a fix. However, I checked and it actually doesn't fix the issue :-/

The actual issue is what @jannisbosch mentioned, the first / in /C:/.... Like I said the url.fileURLToPath solves this issue.

Here's my pull request: https://github.com/bjoluc/jspsych-builder/pull/12#issue-1078661998

bjoluc commented 2 years ago

Thanks @Haffi921!

Obviously, I don't know the whole scope of why you changed from path.resolve to the URL so you need to evaluate this yourself :)

Simply because __dirname is not available in ES6 modules, only import.meta.url...

Haffi921 commented 2 years ago

Yeah exactly, I saw that too. It's been a long time since I've even had a use for dirname myself, so I am not completely involved.

But good news is, as I've managed to understand it, url.fileURLToPath(import.meta.url) seems to produce the equivalent of __dirname + the filename, I think.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 4.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

jannisbosch commented 2 years ago

Thanks guys for fixing this! I think I learned some stuff by following your discussions. Looking forward to trying out JATOS now ;)