Skip to content

Commit 7fd30c1

Browse files
committed
fix(module): unify IsLikelyOptionalModule and exclude explicit file extensions
* IsLikelyOptionalModule existed as two separate, independently-maintained copies — one driving require() in ModuleInternal.mm, one driving import() in ModuleInternalCallbacks.mm — that had already drifted out of sync (only the ESM copy excluded "node:" specifiers). Unified into a single definition declared in ModuleInternal.h so require() and import() can't diverge again. * The heuristic treated any bare specifier as "likely optional" purely by the absence of a leading '.'/'~'/'/', so a real missing file reference like require('foo.js') silently got a lazily-throwing placeholder instead of failing at require()/import() time. This is what broke "should throw error if cant find node module": 6e6adc8 correctly fixed the placeholder's script-injection bug, which removed an accidental compile failure that used to (mis)produce the throw this test depends on. * Bare specifiers ending in a recognized file extension (.js/.mjs/.cjs/ .json/.node/.ts) are now excluded from the optional-module heuristic — real npm package names don't carry a file extension — so require()/ import() throw immediately for those, matching Node's require() semantics, without touching the extensionless case the placeholder exists for. * Added require()/import() parity tests pinning both sides of the boundary: a dotted-but-not-extension name (lodash.debounce-shaped) still defers to the placeholder; extension-qualified names — including the real "video.js" package, called out explicitly as the known trade-off — throw/reject immediately; node: specifiers via require() always hard-fail.
1 parent 260259c commit 7fd30c1

4 files changed

Lines changed: 86 additions & 16 deletions

File tree

‎NativeScript/runtime/ModuleInternal.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,14 @@
66

77
namespace tns {
88

9+
// Heuristic: does this bare specifier look like an npm-style package name
10+
// (as opposed to an explicit, extension-qualified file reference)? Shared by
11+
// both the CommonJS require() path (ModuleInternal.mm) and the ESM
12+
// import()/resolver path (ModuleInternalCallbacks.mm) so the two can't drift
13+
// out of sync with each other. See the definition in ModuleInternal.mm for
14+
// the full rationale.
15+
bool IsLikelyOptionalModule(const std::string& moduleName);
16+
917
class ModuleInternal {
1018
public:
1119
ModuleInternal(v8::Local<v8::Context> context);

‎NativeScript/runtime/ModuleInternal.mm‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,41 @@
1919

2020
namespace tns {
2121

22-
// Helper function to check if a module name looks like an optional external module
22+
// Shared by both the require() path (this file) and the import()/ESM resolver path
23+
// (ModuleInternalCallbacks.mm) — see the declaration in ModuleInternal.h. Keeping a single
24+
// definition means a boundary fix like the extension carve-out below automatically applies
25+
// to both call sites instead of only the one that happened to get touched.
2326
bool IsLikelyOptionalModule(const std::string& moduleName) {
24-
// Check if it's a bare module name (no path separators) that could be an npm package
27+
// Node built-ins are handled by their own dedicated resolution path; never treat them as
28+
// an optional external module.
29+
if (moduleName.rfind("node:", 0) == 0) {
30+
return false;
31+
}
32+
33+
// Check if it's a bare module name (no path separators) that could be an npm package.
34+
//
35+
// Bare specifiers that end in a recognizable script/data extension (e.g. "foo.js",
36+
// "config.json") are explicit file references, not npm-style package names — real npm
37+
// package names don't carry a file extension. Treating them as "likely optional" would
38+
// swallow a genuine "module not found" failure behind a lazily-throwing placeholder
39+
// instead of letting require()/import() fail immediately, which is what callers (and the
40+
// existing "should throw error if cant find node module" test) expect for those names.
41+
//
42+
// This carve-out is deliberately narrow: a dotted bare name that doesn't end in one of
43+
// these exact extensions (e.g. "lodash.debounce") is still treated as optional, same as
44+
// before. See ModuleInternal.mm/ModuleInternalCallbacks.mm optional-module tests for the
45+
// cases this boundary is expected to hold for.
46+
static const char* kExplicitFileExtensions[] = {".js", ".mjs", ".cjs", ".json", ".node", ".ts"};
47+
2548
if (moduleName.find('/') == std::string::npos && moduleName.find('\\') == std::string::npos &&
2649
moduleName[0] != '.' && moduleName[0] != '~' && moduleName[0] != '/') {
50+
for (const char* ext : kExplicitFileExtensions) {
51+
size_t extLen = strlen(ext);
52+
if (moduleName.size() > extLen &&
53+
moduleName.compare(moduleName.size() - extLen, extLen, ext) == 0) {
54+
return false;
55+
}
56+
}
2757
return true;
2858
}
2959
return false;

‎NativeScript/runtime/ModuleInternalCallbacks.mm‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,20 +28,8 @@
2828

2929
namespace tns {
3030

31-
// Helper function to check if a module name looks like an optional external module
32-
static bool IsLikelyOptionalModule(const std::string& moduleName) {
33-
// Skip Node.js built-in modules (they should be handled separately)
34-
if (moduleName.rfind("node:", 0) == 0) {
35-
return false;
36-
}
37-
38-
// Check if it's a bare module name (no path separators) that could be an npm package
39-
if (moduleName.find('/') == std::string::npos && moduleName.find('\\') == std::string::npos &&
40-
moduleName[0] != '.' && moduleName[0] != '~' && moduleName[0] != '/') {
41-
return true;
42-
}
43-
return false;
44-
}
31+
// IsLikelyOptionalModule is declared in ModuleInternal.h and defined once in
32+
// ModuleInternal.mm, shared with the require() path so the two can't drift apart.
4533

4634
// Helper function to check if a module name is a Node.js built-in module
4735
static bool IsNodeBuiltinModule(const std::string& moduleName) {

‎TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,50 @@ describe("Node built-in and optional module resolution", function () {
3737
expect(threw).toBe(true);
3838
});
3939

40+
// IsLikelyOptionalModule is shared between require() (CommonJS) and this ESM resolver
41+
// (see ModuleInternal.h) so the boundary can't drift apart between the two paths. The
42+
// require()-side cases live in shared/Require/index.js; these mirror them for import()
43+
// to confirm the two stay in parity.
44+
it("still creates a placeholder for a dotted-but-not-extension bare specifier", async function () {
45+
// A dot that isn't a recognized file extension (e.g. an npm name like "lodash.debounce")
46+
// must still be treated as optional, not as an explicit missing file.
47+
const mod = await import("__ns_optional_test_module.dotted__");
48+
49+
expect(mod).toBeDefined();
50+
expect(typeof mod.default).toBe("object");
51+
52+
let threw = false;
53+
try {
54+
// eslint-disable-next-line no-unused-expressions
55+
mod.default.someProperty;
56+
} catch (e) {
57+
threw = true;
58+
}
59+
expect(threw).toBe(true);
60+
});
61+
62+
it("rejects immediately for a missing bare specifier that carries an explicit file extension", async function () {
63+
// Mirrors the require()-side "video.js" case: a bare specifier with a recognized file
64+
// extension is an explicit (here, missing) file reference, not an optional dependency,
65+
// so import() must reject instead of resolving to a lazily-throwing placeholder.
66+
const names = [
67+
"__ns_missing_import_test__.js",
68+
"__ns_missing_import_test__.json",
69+
"__ns_missing_import_test__.mjs",
70+
"video.js",
71+
];
72+
73+
for (const name of names) {
74+
let threw = false;
75+
try {
76+
await import(name);
77+
} catch (e) {
78+
threw = true;
79+
}
80+
expect(threw).toBe(true);
81+
}
82+
});
83+
4084
it("resolves import-map vendor modules through the explicit vendor registry", async function () {
4185
const configureRuntime = globalThis.__NS_DEV__ && globalThis.__NS_DEV__.configureRuntime;
4286
expect(typeof configureRuntime).toBe("function");

0 commit comments

Comments
 (0)