-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use require.resolve
shim instead of hard-coded paths
#493
use require.resolve
shim instead of hard-coded paths
#493
Conversation
5ba4357
to
12b1f86
Compare
cc @ guybedford |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing - I've commented the necessary change to make this work.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rerun succeeded.
12b1f86
to
6df58e0
Compare
6df58e0
to
0d39a3f
Compare
@guybedford fixed now (details). please let me know if you have any other suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, although I think we can just use the direct approach here to also remain compatible with custom aliasing.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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_modules
directories.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.