Skip to content

Commit

Permalink
More incremental impl of new module registry
Browse files Browse the repository at this point in the history
Starts adding api tests, fixing up some behaviors.
  • Loading branch information
jasnell committed Sep 9, 2024
1 parent 35a35c3 commit ba6f6f5
Show file tree
Hide file tree
Showing 8 changed files with 273 additions and 94 deletions.
6 changes: 6 additions & 0 deletions src/workerd/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -534,3 +534,9 @@ wd_test(
"//conditions:default": ["@platforms//:incompatible"],
}),
)

wd_test(
src = "tests/new-module-registry-test.wd-test",
args = ["--experimental"],
data = ["tests/new-module-registry-test.js"],
)
120 changes: 120 additions & 0 deletions src/workerd/api/tests/new-module-registry-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import { notStrictEqual, ok, rejects, strictEqual } from 'assert';
import { foo, default as def } from 'foo';
import { default as fs } from 'node:fs';
const { foo: foo2, default: def2 } = await import('bar');

// Verify that import.meta.url is correct here.
strictEqual(import.meta.url, 'file:///worker');

// Verify that import.meta.main is true here.
ok(import.meta.main);

// Verify that import.meta.resolve provides correct results here.
// The input should be interpreted as a URL and normalized according
// to the rules in the WHATWG URL specification.
strictEqual(import.meta.resolve('./.././test/.././../foo'), 'file:///foo');

// There are four tests at this top level... one for the import of the node:assert
// module without the node: prefix specifier, two for the imports of the foo and
// bar modules from the worker, and one for the aliases node:fs module from the
// module worker.

strictEqual(foo, 1);
strictEqual(def, 2);
strictEqual(foo2, 1);
strictEqual(def2, 2);
strictEqual(fs, 'abc');

// Equivalent to the above, but using the file: URL scheme.
import { foo as foo3, default as def3 } from 'file:///foo';
strictEqual(foo, foo3);
strictEqual(def, def3);

// Verify that a module is unable to perform IO operations at the top level, even if
// the dynamic import is initiated within the scope of an active IoContext.
export const noTopLevelIo = {
async test() {
await rejects(import('bad'), {
message: /^Disallowed operation called within global scope/,
});
},
};

// Verify that async local storage is propagated into dynamic imports.
export const alsPropagationDynamicImport = {
async test() {
const { AsyncLocalStorage } = await import('async_hooks');
globalThis.als = new AsyncLocalStorage();
const res = await globalThis.als.run(123, () => import('als'));
strictEqual(res.default, 123);
},
};

// Query strings and fragments create new instances of known modules.
export const queryAndFragment = {
async test() {
// Each resolves the same underlying module but creates a new instance.
// The exports should be the same but the module namespaces should be different.

const a = await import('foo?query');
const b = await import('foo#fragment');
const c = await import('foo?query#fragment');
const d = await import('foo');

strictEqual(a.default, 2);
strictEqual(a.foo, 1);
strictEqual(a.default, b.default);
strictEqual(a.default, c.default);
strictEqual(a.default, d.default);
strictEqual(a.foo, b.foo);
strictEqual(a.foo, c.foo);
strictEqual(a.foo, d.foo);

notStrictEqual(a, b);
notStrictEqual(a, c);
notStrictEqual(a, d);
notStrictEqual(b, c);
notStrictEqual(b, d);
notStrictEqual(c, d);

// The import.meta.url for each should match the specifier used to import the instance.
strictEqual(a.bar, 'file:///foo?query');
strictEqual(b.bar, 'file:///foo#fragment');
strictEqual(c.bar, 'file:///foo?query#fragment');
strictEqual(d.bar, 'file:///foo');
},
};

// TODO(now): Tests
// * [ ] Include tests for all known module types
// * [ ] ESM
// * [ ] CommonJS
// * [ ] Text
// * [ ] Data
// * [ ] JSON
// * [ ] WASM
// * [ ] Python
// * [x] IO is forbidden in top-level module scope
// * [x] Async local storage context is propagated into dynamic imports
// * [ ] require(...) Works in CommonJs Modules
// * [x] Static import correctly handles node: modules with/without the node: prefix
// * [x] Dynamic import correctly handles node: modules with/without the node: prefix
// * [x] Worker bundle can alias node: modules
// * [ ] require(...) correctly handles node: modules with/without the node: prefix
// * [ ] modules not found are correctly reported as errors
// * [ ] Circular dependencies are correctly handled
// * [ ] Errors during ESM evaluation are correctly reported
// * [ ] Errors during CommonJs evaluation are correctly reported
// * [ ] Errors during dynamic import are correctly reported
// * [ ] Errors in JSON module parsing are correctly reported
// * [ ] ESM with no default export is correctly reported as error
// * [ ] CommonJs modules correctly expose named exports
// * [ ] require('module').createRequire API works as expected
// * [ ] Fallback service works as expected
// * [ ] Module specifiers are correctly handled as URLs
// * [ ] Querys and fragments resolve new instances of known modules
// * [ ] URL resolution works correctly
// * [ ] Invalid URLs are correctly reported as errors
// * [ ] Import assertions should be rejected
// * [ ] console.log output correctly uses node-internal:inspect for output
// ...
29 changes: 29 additions & 0 deletions src/workerd/api/tests/new-module-registry-test.wd-test
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
services = [
( name = "new-module-registry-test",
worker = (
modules = [
(name = "worker", esModule = embed "new-module-registry-test.js"),
(name = "foo", esModule = "export const foo = 1; export default 2; export const bar = import.meta.url"),
(name = "bar", esModule = "export const foo = 1; export default 2;"),
(name = "node:fs", esModule = "export default 'abc'"),

# Intentionally bad module to test error handling.
# Evaluation will error because i/o is not permitted at top-level scope.
(name = "bad", esModule = "export default 1; setTimeout(() => {}, 10)"),

# Ensure that async context is propagated into a dynamic import.
(name = "als", esModule = "export default globalThis.als.getStore()"),
],
compatibilityDate = "2024-07-01",
compatibilityFlags = [
"nodejs_compat_v2",
"new_module_registry",
"experimental",
],
)
),
],
);
140 changes: 76 additions & 64 deletions src/workerd/jsg/modules-new.c++
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,14 @@ v8::MaybeLocal<v8::Promise> dynamicImport(v8::Local<v8::Context> context,
return KJ_ASSERT_NONNULL(Url::tryParse(str.asPtr()));
})();

// If Node.js Compat v2 mode is enable, we have to check to see if the specifier
// is a bare node specifier and resolve it to a full node: URL.
if (isNodeJsCompatEnabled(js)) {
KJ_IF_SOME(nodeSpec, checkNodeSpecifier(spec)) {
spec = kj::mv(nodeSpec);
}
}

KJ_IF_SOME(url, referrer.tryResolve(spec.asPtr())) {
auto normalized = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
auto& registry = IsolateModuleRegistry::from(isolate);
Expand Down Expand Up @@ -706,76 +714,80 @@ v8::MaybeLocal<v8::Module> resolveCallback(v8::Local<v8::Context> context,
auto& registry = IsolateModuleRegistry::from(isolate);
auto& js = Lock::from(isolate);

try {
return js.tryCatch([&]() -> v8::MaybeLocal<v8::Module> {
auto spec = kj::str(specifier);

// The proposed specification for import assertions strongly recommends that
// embedders reject import attributes and types they do not understand/implement.
// This is because import attributes can alter the interpretation of a module.
// Throwing an error for things we do not understand is the safest thing to do
// for backwards compatibility.
//
// For now, we do not support any import attributes, so if there are any at all
// we will reject the import.
if (!import_assertions.IsEmpty()) {
if (import_assertions->Length() > 0) {
js.throwException(js.typeError("Import attributes are not supported"));
};
}
return js.tryCatch([&]() -> v8::MaybeLocal<v8::Module> {
auto spec = kj::str(specifier);

// The proposed specification for import assertions strongly recommends that
// embedders reject import attributes and types they do not understand/implement.
// This is because import attributes can alter the interpretation of a module.
// Throwing an error for things we do not understand is the safest thing to do
// for backwards compatibility.
//
// For now, we do not support any import attributes, so if there are any at all
// we will reject the import.
if (!import_assertions.IsEmpty()) {
if (import_assertions->Length() > 0) {
js.throwException(js.typeError("Import attributes are not supported"));
};
}

ResolveContext::Type type = ResolveContext::Type::BUNDLE;
ResolveContext::Type type = ResolveContext::Type::BUNDLE;

auto& referrerUrl = registry.lookup(js, referrer)
.map([&](IsolateModuleRegistry::Entry& entry) -> const Url& {
switch (entry.module.type()) {
case Module::Type::BUNDLE: {
type = ResolveContext::Type::BUNDLE;
break;
}
case Module::Type::BUILTIN: {
type = ResolveContext::Type::BUILTIN;
break;
}
case Module::Type::BUILTIN_ONLY: {
type = ResolveContext::Type::BUILTIN_ONLY;
break;
}
case Module::Type::FALLBACK: {
type = ResolveContext::Type::BUNDLE;
break;
}
auto& referrerUrl = registry.lookup(js, referrer)
.map([&](IsolateModuleRegistry::Entry& entry) -> const Url& {
switch (entry.module.type()) {
case Module::Type::BUNDLE: {
type = ResolveContext::Type::BUNDLE;
break;
}
return entry.specifier.specifier;
}).orDefault(ModuleBundle::BundleBuilder::BASE);

KJ_IF_SOME(url, referrerUrl.tryResolve(spec)) {
// Make sure that percent-encoding in the path is normalized so we can match correctly.
auto normalized = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
ResolveContext resolveContext = {
.type = type,
.source = ResolveContext::Source::STATIC_IMPORT,
.specifier = normalized,
.referrer = referrerUrl,
.rawSpecifier = spec.asPtr(),
};
// TODO(soon): Add import assertions to the context.
case Module::Type::BUILTIN: {
type = ResolveContext::Type::BUILTIN;
break;
}
case Module::Type::BUILTIN_ONLY: {
type = ResolveContext::Type::BUILTIN_ONLY;
break;
}
case Module::Type::FALLBACK: {
type = ResolveContext::Type::BUNDLE;
break;
}
}
return entry.specifier.specifier;
}).orDefault(ModuleBundle::BundleBuilder::BASE);

return registry.resolve(js, resolveContext);
// If Node.js Compat v2 mode is enable, we have to check to see if the specifier
// is a bare node specifier and resolve it to a full node: URL.
if (isNodeJsCompatEnabled(js)) {
KJ_IF_SOME(nodeSpec, checkNodeSpecifier(spec)) {
spec = kj::mv(nodeSpec);
}
}

js.throwException(js.error(kj::str("Invalid module specifier: "_kj, specifier)));
}, [&](Value exception) -> v8::MaybeLocal<v8::Module> {
// If there are any synchronously thrown exceptions, we want to catch them
// here and convert them into a rejected promise. The only exception are
// fatal cases where the isolate is terminating which won't make it here
// anyway.
js.v8Isolate->ThrowException(exception.getHandle(js));
return v8::MaybeLocal<v8::Module>();
});
} catch (...) {
kj::throwFatalException(kj::getCaughtExceptionAsKj());
}
KJ_IF_SOME(url, referrerUrl.tryResolve(spec)) {
// Make sure that percent-encoding in the path is normalized so we can match correctly.
auto normalized = url.clone(Url::EquivalenceOption::NORMALIZE_PATH);
ResolveContext resolveContext = {
.type = type,
.source = ResolveContext::Source::STATIC_IMPORT,
.specifier = normalized,
.referrer = referrerUrl,
.rawSpecifier = spec.asPtr(),
};
// TODO(soon): Add import assertions to the context.

return registry.resolve(js, resolveContext);
}

js.throwException(js.error(kj::str("Invalid module specifier: "_kj, specifier)));
}, [&](Value exception) -> v8::MaybeLocal<v8::Module> {
// If there are any synchronously thrown exceptions, we want to catch them
// here and convert them into a rejected promise. The only exception are
// fatal cases where the isolate is terminating which won't make it here
// anyway.
js.v8Isolate->ThrowException(exception.getHandle(js));
return v8::MaybeLocal<v8::Module>();
});
}

// The fallback module bundle calls a single resolve callback to resolve all modules
Expand Down
27 changes: 0 additions & 27 deletions src/workerd/jsg/modules.c++
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,6 @@
namespace workerd::jsg {
namespace {

// This list must be kept in sync with the list of builtins from Node.js.
// It should be unlikely that anything is ever removed from this list, and
// adding items to it is considered a semver-major change in Node.js.
static const std::set<kj::StringPtr> NODEJS_BUILTINS{"_http_agent", "_http_client", "_http_common",
"_http_incoming", "_http_outgoing", "_http_server", "_stream_duplex", "_stream_passthrough",
"_stream_readable", "_stream_transform", "_stream_wrap", "_stream_writable", "_tls_common",
"_tls_wrap", "assert", "assert/strict", "async_hooks", "buffer", "child_process", "cluster",
"console", "constants", "crypto", "dgram", "diagnostics_channel", "dns", "dns/promises", "domain",
"events", "fs", "fs/promises", "http", "http2", "https", "inspector", "inspector/promises",
"module", "net", "os", "path", "path/posix", "path/win32", "perf_hooks", "process", "punycode",
"querystring", "readline", "readline/promises", "repl", "stream", "stream/consumers",
"stream/promises", "stream/web", "string_decoder", "sys", "timers", "timers/promises", "tls",
"trace_events", "tty", "url", "util", "util/types", "v8", "vm", "wasi", "worker_threads", "zlib"};

// The CompileCache is used to hold cached compilation data for built-in JavaScript modules.
//
// Importantly, this is a process-lifetime in-memory cache that is only appropriate for
Expand Down Expand Up @@ -278,19 +264,6 @@ v8::MaybeLocal<v8::Value> evaluateSyntheticModuleCallback(

} // namespace

kj::Maybe<kj::String> checkNodeSpecifier(kj::StringPtr specifier) {
if (NODEJS_BUILTINS.contains(specifier)) {
return kj::str("node:", specifier);
} else if (specifier.startsWith("node:")) {
return kj::str(specifier);
}
return kj::none;
}

bool isNodeJsCompatEnabled(jsg::Lock& js) {
return IsolateBase::from(js.v8Isolate).isNodeJsCompatEnabled();
}

ModuleRegistry* getModulesForResolveCallback(v8::Isolate* isolate) {
return static_cast<ModuleRegistry*>(
isolate->GetCurrentContext()->GetAlignedPointerFromEmbedderData(2));
Expand Down
3 changes: 0 additions & 3 deletions src/workerd/jsg/modules.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@

namespace workerd::jsg {

kj::Maybe<kj::String> checkNodeSpecifier(kj::StringPtr specifier);
bool isNodeJsCompatEnabled(jsg::Lock& js);

class CommonJsModuleContext;

class CommonJsModuleObject: public jsg::Object {
Expand Down
Loading

0 comments on commit ba6f6f5

Please sign in to comment.