Skip to content

Commit 148f755

Browse files
mikeamikea
authored
Revert "Continue work on new module registry (#3372)" (#3401)
This reverts commit da66065.
1 parent 83770a8 commit 148f755

File tree

7 files changed

+111
-244
lines changed

7 files changed

+111
-244
lines changed

‎src/workerd/api/tests/new-module-registry-test.js

+3-25
Original file line numberDiff line numberDiff line change
@@ -46,18 +46,6 @@ strictEqual(Buffer.from(data.default).toString(), 'abcdef');
4646
import * as json from 'json';
4747
deepStrictEqual(json.default, { foo: 1 });
4848

49-
import * as cjs from 'cjs';
50-
deepStrictEqual(cjs.default, 1);
51-
52-
import * as nodejs from 'nodejs';
53-
deepStrictEqual(nodejs.default, 2);
54-
55-
import { a as cjsA } from 'cjs2';
56-
deepStrictEqual(cjsA, 1);
57-
58-
import * as cjs3 from 'cjs3';
59-
deepStrictEqual(cjs.default, 1);
60-
6149
await rejects(import('invalid-json'), {
6250
message: /Unexpected non-whitespace character after JSON/,
6351
});
@@ -153,20 +141,10 @@ export const evalErrorsInEsmTopLevel = {
153141
},
154142
};
155143

156-
export const cjsThrow = {
157-
async test() {
158-
await rejects(import('cjs-throw'), {
159-
message: 'boom',
160-
});
161-
},
162-
};
163-
164144
// TODO(now): Tests
165145
// * [ ] Include tests for all known module types
166146
// * [x] ESM
167-
// * [x] CommonJS
168-
// * [x] CommonJS with top level errors
169-
// * [ ] CommonJS with disallowed top-level i/o
147+
// * [ ] CommonJS
170148
// * [x] Text
171149
// * [x] Data
172150
// * [x] JSON
@@ -186,12 +164,12 @@ export const cjsThrow = {
186164
// * [x] URL resolution works correctly
187165
// * [x] Invalid URLs are correctly reported as errors
188166
// * [x] Import assertions should be rejected
189-
// * [x] require(...) Works in CommonJs Modules
167+
// * [ ] require(...) Works in CommonJs Modules
190168
// * [ ] require(...) correctly handles node: modules with/without the node: prefix
191169
// * [ ] Circular dependencies are correctly handled
192170
// * [ ] Errors during CommonJs evaluation are correctly reported
193171
// * [ ] Entry point ESM with no default export is correctly reported as error
194-
// * [x] CommonJs modules correctly expose named exports
172+
// * [ ] CommonJs modules correctly expose named exports
195173
// * [ ] require('module').createRequire API works as expected
196174
// * [ ] Fallback service works as expected
197175
// * [ ] console.log output correctly uses node-internal:inspect for output

‎src/workerd/api/tests/new-module-registry-test.wd-test

-9
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,6 @@ const unitTests :Workerd.Config = (
3030
(name = "data", data = "abcdef"),
3131
(name = "json", json = "{ \"foo\": 1 }"),
3232
(name = "invalid-json", json = "1n"),
33-
(name = "cjs", commonJsModule = "module.exports = 1"),
34-
(name = "cjs-throw", commonJsModule = "throw new Error('boom')"),
35-
(name = "nodejs", nodeJsCompatModule = "module.exports = 2"),
36-
(name = "cjs2",
37-
commonJsModule = "module.exports = { a: 1 }",
38-
namedExports = ["a"],
39-
),
40-
(name = "cjs3",
41-
commonJsModule = "module.exports = require('.././.././cjs')")
4233
],
4334
compatibilityDate = "2024-07-01",
4435
compatibilityFlags = [

‎src/workerd/jsg/commonjs.c++

+81-167
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
#include "commonjs.h"
22

3-
#include "modules-new.h"
43
#include "modules.h"
54

65
namespace workerd::jsg {
@@ -10,95 +9,55 @@ CommonJsModuleContext::CommonJsModuleContext(jsg::Lock& js, kj::Path path)
109
path(kj::mv(path)),
1110
exports(js.v8Isolate, module->getExports(js)) {}
1211

13-
CommonJsModuleContext::CommonJsModuleContext(jsg::Lock& js, const jsg::Url& url)
14-
: module(jsg::alloc<CommonJsModuleObject>(js, kj::str(url.getHref()))),
15-
path(url.clone()),
16-
exports(js.v8Isolate, module->getExports(js)) {}
17-
1812
v8::Local<v8::Value> CommonJsModuleContext::require(jsg::Lock& js, kj::String specifier) {
19-
KJ_SWITCH_ONEOF(path) {
20-
KJ_CASE_ONEOF(p, kj::Path) {
21-
auto modulesForResolveCallback = getModulesForResolveCallback(js.v8Isolate);
22-
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");
23-
24-
if (isNodeJsCompatEnabled(js)) {
25-
KJ_IF_SOME(nodeSpec, checkNodeSpecifier(specifier)) {
26-
specifier = kj::mv(nodeSpec);
27-
}
28-
}
29-
30-
kj::Path targetPath = ([&] {
31-
// If the specifier begins with one of our known prefixes, let's not resolve
32-
// it against the referrer.
33-
if (specifier.startsWith("node:") || specifier.startsWith("cloudflare:") ||
34-
specifier.startsWith("workerd:")) {
35-
return kj::Path::parse(specifier);
36-
}
37-
return p.parent().eval(specifier);
38-
})();
39-
40-
// require() is only exposed to worker bundle modules so the resolve here is only
41-
// permitted to require worker bundle or built-in modules. Internal modules are
42-
// excluded.
43-
auto& info =
44-
JSG_REQUIRE_NONNULL(modulesForResolveCallback->resolve(js, targetPath, p,
45-
ModuleRegistry::ResolveOption::DEFAULT,
46-
ModuleRegistry::ResolveMethod::REQUIRE, specifier.asPtr()),
47-
Error, "No such module \"", targetPath.toString(), "\".");
48-
// Adding imported from suffix here not necessary like it is for
49-
// resolveCallback, since we have a js stack that will include the parent
50-
// module's name and location of the failed require().
51-
52-
ModuleRegistry::RequireImplOptions options = ModuleRegistry::RequireImplOptions::DEFAULT;
53-
if (getCommonJsExportDefault(js.v8Isolate)) {
54-
options = ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT;
55-
}
56-
57-
return ModuleRegistry::requireImpl(js, info, options);
58-
}
59-
KJ_CASE_ONEOF(u, jsg::Url) {
60-
return modules::ModuleRegistry::resolve(js, specifier, "default"_kj,
61-
modules::ResolveContext::Type::BUNDLE, modules::ResolveContext::Source::REQUIRE, u);
13+
auto modulesForResolveCallback = getModulesForResolveCallback(js.v8Isolate);
14+
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");
15+
16+
if (isNodeJsCompatEnabled(js)) {
17+
KJ_IF_SOME(nodeSpec, checkNodeSpecifier(specifier)) {
18+
specifier = kj::mv(nodeSpec);
6219
}
6320
}
64-
KJ_UNREACHABLE;
21+
22+
kj::Path targetPath = ([&] {
23+
// If the specifier begins with one of our known prefixes, let's not resolve
24+
// it against the referrer.
25+
if (specifier.startsWith("node:") || specifier.startsWith("cloudflare:") ||
26+
specifier.startsWith("workerd:")) {
27+
return kj::Path::parse(specifier);
28+
}
29+
return path.parent().eval(specifier);
30+
})();
31+
32+
// require() is only exposed to worker bundle modules so the resolve here is only
33+
// permitted to require worker bundle or built-in modules. Internal modules are
34+
// excluded.
35+
auto& info = JSG_REQUIRE_NONNULL(modulesForResolveCallback->resolve(js, targetPath, path,
36+
ModuleRegistry::ResolveOption::DEFAULT,
37+
ModuleRegistry::ResolveMethod::REQUIRE, specifier.asPtr()),
38+
Error, "No such module \"", targetPath.toString(), "\".");
39+
// Adding imported from suffix here not necessary like it is for resolveCallback, since we have a
40+
// js stack that will include the parent module's name and location of the failed require().
41+
42+
ModuleRegistry::RequireImplOptions options = ModuleRegistry::RequireImplOptions::DEFAULT;
43+
if (getCommonJsExportDefault(js.v8Isolate)) {
44+
options = ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT;
45+
}
46+
47+
return ModuleRegistry::requireImpl(js, info, options);
6548
}
6649

6750
void CommonJsModuleContext::visitForMemoryInfo(MemoryTracker& tracker) const {
6851
tracker.trackField("exports", exports);
69-
KJ_SWITCH_ONEOF(path) {
70-
KJ_CASE_ONEOF(p, kj::Path) {
71-
tracker.trackFieldWithSize("path", p.size());
72-
}
73-
KJ_CASE_ONEOF(u, jsg::Url) {
74-
tracker.trackFieldWithSize("path", u.getHref().size());
75-
}
76-
}
52+
tracker.trackFieldWithSize("path", path.size());
7753
}
7854

7955
kj::String CommonJsModuleContext::getFilename() const {
80-
KJ_SWITCH_ONEOF(path) {
81-
KJ_CASE_ONEOF(p, kj::Path) {
82-
return p.toString(true);
83-
}
84-
KJ_CASE_ONEOF(u, jsg::Url) {
85-
return kj::str(u.getPathname());
86-
}
87-
}
88-
KJ_UNREACHABLE;
56+
return path.toString(true);
8957
}
9058

9159
kj::String CommonJsModuleContext::getDirname() const {
92-
KJ_SWITCH_ONEOF(path) {
93-
KJ_CASE_ONEOF(p, kj::Path) {
94-
return p.parent().toString(true);
95-
}
96-
// TODO(new-module-registry): Appropriately convert file URL into path
97-
KJ_CASE_ONEOF(u, jsg::Url) {
98-
return kj::str(u.getPathname());
99-
}
100-
}
101-
KJ_UNREACHABLE;
60+
return path.parent().toString(true);
10261
}
10362

10463
jsg::Ref<CommonJsModuleObject> CommonJsModuleContext::getModule(jsg::Lock& js) {
@@ -108,7 +67,7 @@ jsg::Ref<CommonJsModuleObject> CommonJsModuleContext::getModule(jsg::Lock& js) {
10867
v8::Local<v8::Value> CommonJsModuleContext::getExports(jsg::Lock& js) const {
10968
return exports.getHandle(js);
11069
}
111-
void CommonJsModuleContext::setExports(jsg::Lock& js, jsg::Value value) {
70+
void CommonJsModuleContext::setExports(jsg::Value value) {
11271
exports = kj::mv(value);
11372
}
11473

@@ -139,66 +98,50 @@ NodeJsModuleContext::NodeJsModuleContext(jsg::Lock& js, kj::Path path)
13998
path(kj::mv(path)),
14099
exports(js.v8Ref(module->getExports(js))) {}
141100

142-
NodeJsModuleContext::NodeJsModuleContext(jsg::Lock& js, const jsg::Url& url)
143-
: module(jsg::alloc<NodeJsModuleObject>(js, kj::str(url.getHref()))),
144-
path(url.clone()),
145-
exports(js.v8Isolate, module->getExports(js)) {}
146-
147101
v8::Local<v8::Value> NodeJsModuleContext::require(jsg::Lock& js, kj::String specifier) {
148-
KJ_SWITCH_ONEOF(path) {
149-
KJ_CASE_ONEOF(p, kj::Path) {
150-
// If it is a bare specifier known to be a Node.js built-in, then prefix the
151-
// specifier with node:
152-
bool isNodeBuiltin = false;
153-
auto resolveOption = jsg::ModuleRegistry::ResolveOption::DEFAULT;
154-
KJ_IF_SOME(spec, checkNodeSpecifier(specifier)) {
155-
specifier = kj::mv(spec);
156-
isNodeBuiltin = true;
157-
resolveOption = jsg::ModuleRegistry::ResolveOption::BUILTIN_ONLY;
158-
}
159-
160-
// TODO(cleanup): This implementation from here on is identical to the
161-
// CommonJsModuleContext::require. We should consolidate these as the
162-
// next step.
163-
164-
auto modulesForResolveCallback = jsg::getModulesForResolveCallback(js.v8Isolate);
165-
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");
166-
167-
kj::Path targetPath = ([&] {
168-
// If the specifier begins with one of our known prefixes, let's not resolve
169-
// it against the referrer.
170-
if (specifier.startsWith("node:") || specifier.startsWith("cloudflare:") ||
171-
specifier.startsWith("workerd:")) {
172-
return kj::Path::parse(specifier);
173-
}
174-
return p.parent().eval(specifier);
175-
})();
176-
177-
// require() is only exposed to worker bundle modules so the resolve here is only
178-
// permitted to require worker bundle or built-in modules. Internal modules are
179-
// excluded.
180-
auto& info =
181-
JSG_REQUIRE_NONNULL(modulesForResolveCallback->resolve(js, targetPath, p, resolveOption,
182-
ModuleRegistry::ResolveMethod::REQUIRE, specifier.asPtr()),
183-
Error, "No such module \"", targetPath.toString(), "\".");
184-
// Adding imported from suffix here not necessary like it is for resolveCallback,
185-
// since we have a js stack that will include the parent module's name and location
186-
// of the failed require().
187-
188-
if (!isNodeBuiltin) {
189-
JSG_REQUIRE_NONNULL(
190-
info.maybeSynthetic, TypeError, "Cannot use require() to import an ES Module.");
191-
}
192-
193-
return ModuleRegistry::requireImpl(
194-
js, info, ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT);
195-
}
196-
KJ_CASE_ONEOF(u, jsg::Url) {
197-
return modules::ModuleRegistry::resolve(js, specifier, "default"_kj,
198-
modules::ResolveContext::Type::BUNDLE, modules::ResolveContext::Source::REQUIRE, u);
199-
}
102+
// If it is a bare specifier known to be a Node.js built-in, then prefix the
103+
// specifier with node:
104+
bool isNodeBuiltin = false;
105+
auto resolveOption = jsg::ModuleRegistry::ResolveOption::DEFAULT;
106+
KJ_IF_SOME(spec, checkNodeSpecifier(specifier)) {
107+
specifier = kj::mv(spec);
108+
isNodeBuiltin = true;
109+
resolveOption = jsg::ModuleRegistry::ResolveOption::BUILTIN_ONLY;
200110
}
201-
KJ_UNREACHABLE;
111+
112+
// TODO(cleanup): This implementation from here on is identical to the
113+
// CommonJsModuleContext::require. We should consolidate these as the
114+
// next step.
115+
116+
auto modulesForResolveCallback = jsg::getModulesForResolveCallback(js.v8Isolate);
117+
KJ_REQUIRE(modulesForResolveCallback != nullptr, "didn't expect resolveCallback() now");
118+
119+
kj::Path targetPath = ([&] {
120+
// If the specifier begins with one of our known prefixes, let's not resolve
121+
// it against the referrer.
122+
if (specifier.startsWith("node:") || specifier.startsWith("cloudflare:") ||
123+
specifier.startsWith("workerd:")) {
124+
return kj::Path::parse(specifier);
125+
}
126+
return path.parent().eval(specifier);
127+
})();
128+
129+
// require() is only exposed to worker bundle modules so the resolve here is only
130+
// permitted to require worker bundle or built-in modules. Internal modules are
131+
// excluded.
132+
auto& info =
133+
JSG_REQUIRE_NONNULL(modulesForResolveCallback->resolve(js, targetPath, path, resolveOption,
134+
ModuleRegistry::ResolveMethod::REQUIRE, specifier.asPtr()),
135+
Error, "No such module \"", targetPath.toString(), "\".");
136+
// Adding imported from suffix here not necessary like it is for resolveCallback, since we have a
137+
// js stack that will include the parent module's name and location of the failed require().
138+
139+
if (!isNodeBuiltin) {
140+
JSG_REQUIRE_NONNULL(
141+
info.maybeSynthetic, TypeError, "Cannot use require() to import an ES Module.");
142+
}
143+
144+
return ModuleRegistry::requireImpl(js, info, ModuleRegistry::RequireImplOptions::EXPORT_DEFAULT);
202145
}
203146

204147
v8::Local<v8::Value> NodeJsModuleContext::getBuffer(jsg::Lock& js) {
@@ -217,28 +160,11 @@ v8::Local<v8::Value> NodeJsModuleContext::getProcess(jsg::Lock& js) {
217160
}
218161

219162
kj::String NodeJsModuleContext::getFilename() {
220-
KJ_SWITCH_ONEOF(path) {
221-
KJ_CASE_ONEOF(p, kj::Path) {
222-
return p.toString(true);
223-
}
224-
KJ_CASE_ONEOF(u, jsg::Url) {
225-
return kj::str(u.getPathname());
226-
}
227-
}
228-
KJ_UNREACHABLE;
163+
return path.toString(true);
229164
}
230165

231166
kj::String NodeJsModuleContext::getDirname() {
232-
KJ_SWITCH_ONEOF(path) {
233-
KJ_CASE_ONEOF(p, kj::Path) {
234-
return p.parent().toString(true);
235-
}
236-
// TODO(new-module-registry): Appropriately convert file URL into path
237-
KJ_CASE_ONEOF(u, jsg::Url) {
238-
return kj::str(u.getPathname());
239-
}
240-
}
241-
KJ_UNREACHABLE;
167+
return path.parent().toString(true);
242168
}
243169

244170
jsg::Ref<NodeJsModuleObject> NodeJsModuleContext::getModule(jsg::Lock& js) {
@@ -269,16 +195,4 @@ kj::StringPtr NodeJsModuleObject::getPath() {
269195
return path;
270196
}
271197

272-
void NodeJsModuleContext::visitForMemoryInfo(MemoryTracker& tracker) const {
273-
tracker.trackField("exports", exports);
274-
KJ_SWITCH_ONEOF(path) {
275-
KJ_CASE_ONEOF(p, kj::Path) {
276-
tracker.trackFieldWithSize("path", p.size());
277-
}
278-
KJ_CASE_ONEOF(u, jsg::Url) {
279-
tracker.trackFieldWithSize("path", u.getHref().size());
280-
}
281-
}
282-
}
283-
284198
} // namespace workerd::jsg

0 commit comments

Comments
 (0)