-
Notifications
You must be signed in to change notification settings - Fork 30
fix: use custom npm module path for js tests. #796
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
Signed-off-by: Dario Valdespino <[email protected]>
WalkthroughWalkthroughThe recent modifications in the Changes
Assessment against linked issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (1)
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
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.
Fantastic work and very well explained PR; a work of art
Summary
Fixes the import issues in the
testEmbeddedCjs
andtestEmbeddedEsm
cases in theJsPluginTest
suite, caused by an undocumented quirk of the module resolution algorithm (see below for full analysis). The following changes are enough to resolve the issue, note that this is not a bug, it could be considered a user error (albeit one caused by bad ux on our part).modulesPath
configuration, to allow resolving dependencies from the bundle extracted into the guest layer of the VFS.hello.tar.gz
embedded test bundle, moving its contents into anapp
directory, to ensure it is extracted as/app/node_modules/hello
instead of/node_modules/hello
.Diagnostic
The
hello.tar.gz
test bundle contains ahello
module which is used in the guest code under test:elide/packages/graalvm/src/test/kotlin/elide/runtime/plugins/js/JsPluginTest.kt
Lines 107 to 113 in 3138d14
The bundle is extracted to the root of the in-memory vfs layer, resulting in
/node_modules/hello
being the path to the module. But the default path for module search is.
, which causes the GraalVM module resolver to look for./node_modules/hello
during the tests (e.g./workspaces/elide/packages/graalvm/node_modules/hello
), and fail since there is no such path.This is not technically a bug, it is actually an inconsistency resulting from the way in which user bundles are extracted into the in-memory vfs layer. The solution in this case is to adjust the
modulesPath
configuration for the JS plugin and point it to the correct modules root for the in-memory layer (or the host layer, if the dependencies are located in the host FS):elide/packages/graalvm/src/test/kotlin/elide/runtime/plugins/js/JsPluginTest.kt
Lines 51 to 53 in dd63945
Additionally, in the case of loading modules provided by an embedded bundle, special care must be taken to ensure that the target dependencies are never extracted at the root of the file system, for example, by wrapping the
node_modules
directory in another directory, yielding the path/app/node_modules
instead of/node_modules
when extracted. If the modules directory is placed in the root, it will not be recognized by GraalVM's default module loader regardless of themodulesPath
configuration.Recommendations
Given the very specific semantics of module resolution revealed by this issue, we should:
dependencies and paths in general, e.g. allowing users to specify the root path for extracting embedded bundles.
Fixes #790
Summary by CodeRabbit