use require.resolve shim instead of hard-coded paths
#493
Conversation
OmarTawfik
force-pushed
the
fix-require-resolve
branch
from
5ba4357
to
12b1f86
Compare
|
cc @ guybedford |
src/cmd/opt.js
Outdated
| } catch { | ||
| WASM_OPT = new URL('../../node_modules/binaryen/bin/wasm-opt', import.meta.url); | ||
| } | ||
| const WASM_OPT = require.resolve('binaryen/bin/wasm-opt'); |
There was a problem hiding this comment.
It's better to use import.meta.resolve here, see https://github.com/bytecodealliance/jco/blob/main/src/cmd/run.js#L90C57-L90C76 for an example.
There was a problem hiding this comment.
Thanks for the pointer. I changed the PR to use import.meta.resolve, which passes all tests now, with one caveat: import.meta.resolve actually resolves to the source file being imported, not just the directory of the package, which requires us to traverse up to find it. This works correctly for the current inputs, and is an improvement to the current code, but we might need to reconsider if we need to handle more complex inputs.
There was a problem hiding this comment.
There is a single windows-only test failure. I think it is unrelated to this PR (a race condition in a compiled binary), but I'm not able to repro locally. I will try rerunning the CI job.
There was a problem hiding this comment.
Rerun succeeded.
OmarTawfik
force-pushed
the
fix-require-resolve
branch
from
12b1f86
to
6df58e0
Compare
OmarTawfik
force-pushed
the
fix-require-resolve
branch
from
6df58e0
to
0d39a3f
Compare
|
@guybedford fixed now (details). please let me know if you have any other suggestions. |
src/common.js
Outdated
| let result = fileURLToPath(import.meta.resolve(input)); | ||
|
|
||
| const inputBase = basename(input); | ||
| while (inputBase !== basename(result)) { | ||
| const resultDir = dirname(result); | ||
|
|
||
| // Short-circuit if we hit the root: | ||
| assert.notEqual(resultDir, result, `Unable to resolve import '${input}'`); | ||
|
|
||
| result = resultDir; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
This is a little bit brittle since it is incompatible with custom aliasing of paths (assumes the path name matches the package name), so I'd suggest instead just using import.meta.resolve rules directly, rather than having this resolveImport abstraction:
wasm_opt = path.resolve(fileURLToPath(import.meta.resolve('binaryen')), 'bin/wasm2js');
preview2_shims = path.resolve(fileURLToPath(import.meta.resolve('@bytecodealliance/preview2-shims')), '../../');kind of thing.
the existing hard-coded paths fail when the project is installed/ran via a source dependency, instead of `npx`, because `jco`'s own dependencies are installed in parent `node_modules` directories. This change reuses NodeJS's own resolution mechanism to find the correct paths directly, without having to hardcode them.
OmarTawfik
force-pushed
the
fix-require-resolve
branch
from
0d39a3f
to
df0fa03
Compare
|
Thanks for this! |
the existing hard-coded paths fail when the project is installed/ran via a source/git dependency, instead of
npx, becausejco's own dependencies are installed in parentnode_modulesdirectories.This change reuses NodeJS's own resolution mechanism to find the correct paths directly, without having to hardcode them, which should work everywhere.
NOTE: there are two tests failing, because (I suspect) they are using a custom directory layout that doesn't follow
node_modules/*. I wasn't able yet to figure out how they are set up, and I want to make sure this is the right change first.Please let me know if you have any suggestions.