[Bug] Unable to fetch module data with `XMLHttpRequest`

Yongbeom-Kim commented 1 year ago

Problem

Is as title says - I can't fetch module data from https://source-academy.github.io/modules/modules.json from js-slang (times out).

stdout:

C:\Users\Dernbu\Documents\NUS Modules\CP3209_UROP\SourceAcademy\js-slang>node dist/repl/repl.js --chapter=4 "import {entry} from 'binary_tree';" 
(node:109464) [DEP0128] DeprecationWarning: Invalid 'main' field in 'C:\Users\Dernbu\Documents\NUS Modules\CP3209_UROP\SourceAcademy\js-slang\node_modules\node-getopt\package.json' of './lib'. Please either fix that or report it to the module author
(Use `node --trace-deprecation ...` to show where the warning was created)
Unable to get modules.

I have confirmed that this is not an internet issue (I can access the URL with my browser), and I've tried with both NUS_STU wifi and on my mobile hotspot.
I also find that the issue is localised to the xmlhttprequest-ts library, and a test script with axios for a get request on the same website runs fine

Versions:

Windows 11
Node 18.13.0 (I've tried with 17.9.1 and 16.19.0 as well)

Steps to reproduce

js-slang

Git clone js-slang repo
Follow instructions to build
Run node dist/repl/repl.js --chapter=4 "import {entry} from 'binary_tree';"

Expected output: Unable to get modules after timeout

test script

clone this repo
npm run build
node ./dist/index.js

Output:

Error: request timed out after 10000ms
    at XMLHttpRequest3.send (C:\Users\Dernbu\Documents\NUS Modules\CP3209_UROP\SourceAcademy\frontend-ping-test\dist\index.js:408:46)
    at httpGet (C:\Users\Dernbu\Documents\NUS Modules\CP3209_UROP\SourceAcademy\frontend-ping-test\dist\index.js:10542:13)
    at Object.<anonymous> (C:\Users\Dernbu\Documents\NUS Modules\CP3209_UROP\SourceAcademy\frontend-ping-test\dist\index.js:13272:13)
    at Module._compile (node:internal/modules/cjs/loader:1218:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1272:10)
    at Module.load (node:internal/modules/cjs/loader:1081:32)
    at Module._load (node:internal/modules/cjs/loader:922:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47
{
  repeat: { tabs: [ 'Repeat' ] },
  pix_n_flix: { tabs: [ 'Pixnflix' ] },
  binary_tree: { tabs: [] },
  copy_gc: { tabs: [ 'CopyGc' ] },
  curve: { tabs: [ 'Curve' ] },
  mark_sweep: { tabs: [ 'MarkSweep' ] },
  sound: { tabs: [ 'Sound' ] },
  scrabble: { tabs: [] },
  stereo_sound: { tabs: [ 'StereoSound' ] },
  game: { tabs: [ 'Game' ] },
  rune: { tabs: [ 'Rune' ] },
  rune_in_words: { tabs: [] },
  sound_matrix: { tabs: [ 'SoundMatrix' ] },
  csg: { tabs: [ 'Csg' ] }
}

Suggestion:

Replace xmlhttprequest with axios (or fetch API)

Benefits

Fixes this bug
IMO, axios has a much cleaner syntax and easier to manage.

Limitations

Since axios does not support synchronous HTTP requests, we'll need to deal with everything in promises, so probably requires a non-trivial refactor
To pass non-promise values back, requires top-level await, meaning we will need to bump tsconfig to:
```
{
"compilerOptions": {
"module": "esnext" (or system),
"target": "es2017",
}
```

shenyih0ng commented 1 year ago

@Yongbeom-Kim i wasn't able to reproduce this on my system (macos ventura 13.0, m1 chip) with node@18.13.0.

there was no timeout or errors on my end when running the test script:

repl works fine as well:

also i realised that esbuild was used in the test script instead of tsc for compilation. but i don't think it matters tho

let me know which commit you were testing it on so i can test it again

Yongbeom-Kim commented 1 year ago

I've been testing on the latest commit!

I've tried it on Ubuntu WSL (22.04 LTS) and it works, just not on Windows 11. So maybe a unix-dependent thing.

I see that this is a platform-dependent thing, but I'm happy enough that it works... I guess? I see that this is probably an upstream issue with the xmlhttprequest-ts, and so will be closing this issue. I think it should still be replaced with a more modern (say, fetch API), so I'll move this on to a proposal instead

source-academy / js-slang