-
Notifications
You must be signed in to change notification settings - Fork 471
Reliably determine @rescript/runtime path #7637
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
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
BTW this approach should also make it easier to move the runtime to |
@zth This probably needs to be taken into account in the editor extension, too?
So maybe the variable should actually be |
a18e180
to
2daa984
Compare
@cknitt this means that you have to supply that env variable always to be able to run bsc standalone...? |
Yes. I mean we could fall back to the previous algorithm if the variable is not set, but we need to be aware that there are cases where this algorithm will not work. We could also use a command line argument instead of an env var if that's your concern - just more work to implement to get it passed through for both rewatch and bsb. |
Would like to have a fallback for this. I frequently run bsc. |
Note that that would only be an issue when directly running the platform-specific When running bsc via |
b7afc8e
to
56bb638
Compare
That includes running |
Would it be possible to make it an optional bsc arg (that uses the old algorithm if not supplied) and bake resolving that into the "compiler args" command? Rewatch could call a node script to get the path. Maybe in the future we could check if this resolution is needed at all by checking what package manager is used. Bsc is already used standalone in a few places, and I envision it being used even more standalone in the future (Svelte integration, one off scripts, code blocks in docs etc). The overhead of calling a node process for each invocation is non-negligible, and the orchestration for figuring out how to call it shouldn't be too difficult (that's why compiler args is so nice). |
Yes, we could make it an optional bsc arg. There actually already exists an optional arg If the flag is not set, the default could be the simple implementation we had earlier that just assumed npm's I would avoid rewatch calling node though, I'd prefer node/bun/whatever calling rewatch via the wrapper script, with the wrapper script passing the correct location of the runtime as implemented in this PR. rewatch can then directly call bsc.exe with the |
d64d7ed
to
0ccec74
Compare
1817c77
to
f6d8303
Compare
Now using a |
pub module_names: AHashSet<String>, | ||
pub deleted_modules: AHashSet<String>, | ||
pub bsc_path: PathBuf, | ||
pub runtime_path: Option<PathBuf>, |
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.
Not 100% sold that this should be part of the BuildState
.
Computing it in compile::compile
is probably fine.
The fact that is is optional is a bit of a smell to make.
Clean doesn't use this.
rewatch/src/build.rs
Outdated
PathBuf::from(&interface_filename).exists() | ||
}; | ||
|
||
let runtime_path = helpers::get_runtime_path(&project_context).ok(); |
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.
Should this not fail? Isn't this destined to throw a more cryptic error in bsc
if it wasn't found?
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.
Well, it falls back to the default (simple) implementation in bsc then, but it can still continue.
When building @rescript/runtime
itself, the resolution will fail for 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.
(.ok
means "toOption", weird naming...)
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.
Might be good to add that somewhere in the code as well, when it is fine to be None
.
}; | ||
|
||
rescript_legacy_path.unwrap_or_else(|_| panic!("Could not find rescript-legacy.exe")) | ||
// Resolve <project> -> rescript |
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.
Could you elaborate in a code comment why you are resolving rescript
package first? This seems like an unnecessary extra step. Why not search for @rescript/runtime
from the get go?
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.
Tried that, but didn't work, probably because there is no direct dependency on the runtime package. Or maybe I was doing something wrong. I can have another look.
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.
No — that may just be how resolve_oxc
and https://nodejs.org/api/esm.html#resolution-algorithm
work. That’s fine, but we should document it.
There’s a tendency to assume oxc_resolver
can solve everything; I’m all for leaning on it when it helps, but we need to understand and document what it can and cannot do.
Let’s leave clear "whys" for future contributors so they understand our decisions and don’t keep getting challenged.
944d91b
to
3683436
Compare
I've confirmed this works perfectly. BTW. Off topic: The rescript npm package declares it has dependency |
Thanks for testing! That's great to hear! However, we'll most probably not keep the oxc_resolver based implementation, see notes here: #7117 (comment) I will get back to you when we have something to test again!
I don't think so. |
Yes, I agree. But, "dependencies" only install "@rescript/runtime" package under "rescript" package's "node_modules" dir. The generated ".js" file can't resolve to there when using pnpm. "@rescript/runtime" need to be installed at the project's "node_modules" dir. So they can be reached by project generated ".js" files. |
Superseded by #7858 |
(updated description after moving the runtime to
@rescript/runtime
and deciding to use a command line arg instead of an env var)To determine the path of the runtime module
@rescript/runtime
in a reliable way that works with all package managers / package directory layouts, we need to use the JS module resolution algorithm.In rewatch, we can use https://github.com/oxc-project/oxc-resolver.
For the legacy build system, we can just use
require.resolve
.This should fix all issues with pnpm not finding the runtime module.