Skip to content
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

Fixup importing built-ins from module "subdirectories" #1682

Merged
merged 2 commits into from
Feb 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/workerd/api/tests/module-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import * as assert from 'a/b/c';
import * as assert3 from 'node:assert';

export const abortcontroller = {
jasnell marked this conversation as resolved.
Show resolved Hide resolved
async test() {
const assert2 = await import('a/b/c');
if (assert !== assert2 && assert !== assert3) {
throw new Error('bad things happened');
}
}
};
17 changes: 17 additions & 0 deletions src/workerd/api/tests/module-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "module-test",
worker = (
modules = [
(name = "worker", esModule = embed "module-test.js"),
(name = "a/b/c",
esModule = "import * as assert from 'node:assert'; await import('node:buffer'); export default assert;"),
],
compatibilityDate = "2023-01-15",
compatibilityFlags = ["nodejs_compat"]
)
),
],
);
37 changes: 32 additions & 5 deletions src/workerd/jsg/modules.c++
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,18 @@ v8::MaybeLocal<v8::Module> resolveCallback(v8::Local<v8::Context> context,
bool internalOnly = ref.type == ModuleRegistry::Type::BUILTIN ||
ref.type == ModuleRegistry::Type::INTERNAL;

kj::Path targetPath = !internalOnly ?
ref.specifier.parent().eval(spec) :
kj::Path::parse(spec);
kj::Path targetPath = ([&] {
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
auto spec = kj::str(specifier);
if (internalOnly ||
spec.startsWith("node:") ||
spec.startsWith("cloudflare:") ||
spec.startsWith("workerd:")) {
return kj::Path::parse(spec);
}
return ref.specifier.parent().eval(spec);
})();

KJ_IF_SOME(resolved, registry->resolve(js, targetPath, ref.specifier,
internalOnly ?
Expand Down Expand Up @@ -245,7 +254,16 @@ v8::Local<v8::Value> CommonJsModuleContext::require(jsg::Lock& js, kj::String sp
auto modulesForResolveCallback = getModulesForResolveCallback(js.v8Isolate);
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");

kj::Path targetPath = path.parent().eval(specifier);
kj::Path targetPath = ([&] {
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
if (specifier.startsWith("node:") ||
specifier.startsWith("cloudflare:") ||
specifier.startsWith("workerd:")) {
return kj::Path::parse(specifier);
}
return path.parent().eval(specifier);
})();

// require() is only exposed to worker bundle modules so the resolve here is only
// permitted to require worker bundle or built-in modules. Internal modules are
Expand Down Expand Up @@ -587,7 +605,16 @@ v8::Local<v8::Value> NodeJsModuleContext::require(jsg::Lock& js, kj::String spec
auto modulesForResolveCallback = jsg::getModulesForResolveCallback(js.v8Isolate);
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");

kj::Path targetPath = path.parent().eval(specifier);
kj::Path targetPath = ([&] {
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
if (specifier.startsWith("node:") ||
specifier.startsWith("cloudflare:") ||
specifier.startsWith("workerd:")) {
jasnell marked this conversation as resolved.
Show resolved Hide resolved
return kj::Path::parse(specifier);
}
return path.parent().eval(specifier);
})();

// require() is only exposed to worker bundle modules so the resolve here is only
// permitted to require worker bundle or built-in modules. Internal modules are
Expand Down
10 changes: 9 additions & 1 deletion src/workerd/jsg/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -825,9 +825,17 @@ v8::MaybeLocal<v8::Promise> dynamicImportCallback(v8::Local<v8::Context> context
})();

auto maybeSpecifierPath = ([&]() -> kj::Maybe<kj::Path> {
// If the specifier begins with one of our known prefixes, let's not resolve
// it against the referrer.
auto spec = kj::str(specifier);
if (spec.startsWith("node:") ||
spec.startsWith("cloudflare:") ||
spec.startsWith("workerd:")) {
return kj::Path::parse(spec);
}
KJ_IF_SOME(referrerPath, maybeReferrerPath) {
try {
return referrerPath.parent().eval(kj::str(specifier));
return referrerPath.parent().eval(spec);
} catch (kj::Exception& ex) {
return kj::none;
}
Expand Down
Loading