From 75ca07f7c3799e8de19fb06ae2b28fd1bcc9123a Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Wed, 16 Jan 2019 03:11:10 +0200 Subject: [PATCH 1/5] esm: Implement "exports" proposal --- src/env.h | 4 + src/module_wrap.cc | 103 ++++++++++++++++-- test/es-module/test-esm-exports.mjs | 15 +++ test/fixtures/node_modules/pkgexports/asdf.js | 1 + .../node_modules/pkgexports/package.json | 6 + test/fixtures/pkgexports-missing.mjs | 3 + test/fixtures/pkgexports.mjs | 2 + 7 files changed, 124 insertions(+), 10 deletions(-) create mode 100644 test/es-module/test-esm-exports.mjs create mode 100644 test/fixtures/node_modules/pkgexports/asdf.js create mode 100644 test/fixtures/node_modules/pkgexports/package.json create mode 100644 test/fixtures/pkgexports-missing.mjs create mode 100644 test/fixtures/pkgexports.mjs diff --git a/src/env.h b/src/env.h index 3e3d41790e..e68f120bf7 100644 --- a/src/env.h +++ b/src/env.h @@ -92,6 +92,7 @@ struct PackageConfig { enum class Exists { Yes, No }; enum class IsValid { Yes, No }; enum class HasMain { Yes, No }; + enum class HasExports { Yes, No }; enum PackageType : uint32_t { None = 0, CommonJS, Module }; const Exists exists; @@ -99,6 +100,9 @@ struct PackageConfig { const HasMain has_main; const std::string main; const PackageType type; + + const HasExports has_exports; + const v8::CopyablePersistentTraits::CopyablePersistent exports; }; } // namespace loader diff --git a/src/module_wrap.cc b/src/module_wrap.cc index e104afb736..b7c0e8bdcf 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -26,7 +26,6 @@ using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; -using v8::Global; using v8::HandleScope; using v8::Integer; using v8::IntegrityLevel; @@ -39,6 +38,7 @@ using v8::Module; using v8::Nothing; using v8::Number; using v8::Object; +using v8::Persistent; using v8::PrimitiveArray; using v8::Promise; using v8::ScriptCompiler; @@ -537,6 +537,7 @@ using Exists = PackageConfig::Exists; using IsValid = PackageConfig::IsValid; using HasMain = PackageConfig::HasMain; using PackageType = PackageConfig::PackageType; +using HasExports = PackageConfig::HasExports; Maybe GetPackageConfig(Environment* env, const std::string& path, @@ -558,7 +559,8 @@ Maybe GetPackageConfig(Environment* env, if (source.IsNothing()) { auto entry = env->package_json_cache.emplace(path, PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", - PackageType::None }); + PackageType::None, HasExports::No, + Persistent() }); return Just(&entry.first->second); } @@ -578,7 +580,8 @@ Maybe GetPackageConfig(Environment* env, !pkg_json_v->ToObject(context).ToLocal(&pkg_json)) { env->package_json_cache.emplace(path, PackageConfig { Exists::Yes, IsValid::No, HasMain::No, "", - PackageType::None }); + PackageType::None, HasExports::No, + Persistent() }); std::string msg = "Invalid JSON in '" + path + "' imported from " + base.ToFilePath(); node::THROW_ERR_INVALID_PACKAGE_CONFIG(env, msg.c_str()); @@ -609,22 +612,23 @@ Maybe GetPackageConfig(Environment* env, } Local exports_v; + Persistent exports; if (pkg_json->Get(env->context(), env->exports_string()).ToLocal(&exports_v) && - (exports_v->IsObject() || exports_v->IsString() || - exports_v->IsBoolean())) { - Global exports; + (exports_v->IsObject() || + (exports_v->IsBoolean() && exports_v->IsFalse()))) { exports.Reset(env->isolate(), exports_v); auto entry = env->package_json_cache.emplace(path, PackageConfig { Exists::Yes, IsValid::Yes, has_main, main_std, - pkg_type }); + pkg_type, HasExports::Yes, exports }); return Just(&entry.first->second); } auto entry = env->package_json_cache.emplace(path, PackageConfig { Exists::Yes, IsValid::Yes, has_main, main_std, - pkg_type }); + pkg_type, HasExports::No, + Persistent() }); return Just(&entry.first->second); } @@ -646,7 +650,8 @@ Maybe GetPackageScopeConfig(Environment* env, if (pjson_url.path() == last_pjson_url.path()) { auto entry = env->package_json_cache.emplace(pjson_url.ToFilePath(), PackageConfig { Exists::No, IsValid::Yes, HasMain::No, "", - PackageType::None }); + PackageType::None, HasExports::No, + Persistent() }); const PackageConfig* pcfg = &entry.first->second; return Just(pcfg); } @@ -772,6 +777,20 @@ Maybe PackageMainResolve(Environment* env, const PackageConfig& pcfg, const URL& base) { if (pcfg.exists == Exists::Yes) { + Local exports = pcfg.exports.Get(env->isolate()); + if (pcfg.has_exports == HasExports::Yes && exports->IsObject()) { + Local exports_obj = exports.As(); + Local dot_string = String::NewFromUtf8(env->isolate(), ".", + v8::NewStringType::kNormal).ToLocalChecked(); + auto dot_main = + exports_obj->Get(env->context(), dot_string).ToLocalChecked(); + if (dot_main->IsString()) { + Utf8Value main_utf8(env->isolate(), dot_main.As()); + std::string main(*main_utf8, main_utf8.length()); + URL main_url("./" + main, pjson_url); + return FinalizeResolution(env, main_url, base); + } + } if (pcfg.has_main == HasMain::Yes) { URL resolved(pcfg.main, pjson_url); const std::string& path = resolved.ToFilePath(); @@ -800,6 +819,65 @@ Maybe PackageMainResolve(Environment* env, return Nothing(); } +Maybe PackageExportsResolve(Environment* env, + const URL& pjson_url, + const std::string& pkg_subpath, + const PackageConfig& pcfg, + const URL& base) { + Isolate* isolate = env->isolate(); + Local context = env->context(); + Local exports = pcfg.exports.Get(isolate); + if (exports->IsObject()) { + Local exports_obj = exports.As(); + Local subpath = String::NewFromUtf8(isolate, + pkg_subpath.c_str(), v8::NewStringType::kNormal).ToLocalChecked(); + + auto target = exports_obj->Get(context, subpath).ToLocalChecked(); + if (target->IsString()) { + Utf8Value target_utf8(isolate, target.As()); + std::string target(*target_utf8, target_utf8.length()); + if (target.substr(0, 2) == "./") { + URL target_url(target, pjson_url); + return FinalizeResolution(env, target_url, base); + } + } + + Local best_match; + std::string best_match_str = ""; + Local keys = + exports_obj->GetOwnPropertyNames(context).ToLocalChecked(); + for (uint32_t i = 0; i < keys->Length(); ++i) { + Local key = keys->Get(context, i).ToLocalChecked().As(); + Utf8Value key_utf8(isolate, key); + std::string key_str(*key_utf8, key_utf8.length()); + if (key_str.back() != '/') continue; + if (pkg_subpath.substr(0, key_str.length()) == key_str && + key_str.length() > best_match_str.length()) { + best_match = key; + best_match_str = key_str; + } + } + + if (best_match_str.length() > 0) { + auto target = exports_obj->Get(context, best_match).ToLocalChecked(); + if (target->IsString()) { + Utf8Value target_utf8(isolate, target.As()); + std::string target(*target_utf8, target_utf8.length()); + if (target.back() == '/' && target.substr(0, 2) == "./") { + std::string subpath = pkg_subpath.substr(best_match_str.length()); + URL target_url(target + subpath, pjson_url); + return FinalizeResolution(env, target_url, base); + } + } + } + } + std::string msg = "Package exports for '" + + URL(".", pjson_url).ToFilePath() + "' do not define a '" + pkg_subpath + + "' subpath, imported from " + base.ToFilePath(); + node::THROW_ERR_MODULE_NOT_FOUND(env, msg.c_str()); + return Nothing(); +} + Maybe PackageResolve(Environment* env, const std::string& specifier, const URL& base) { @@ -847,7 +925,12 @@ Maybe PackageResolve(Environment* env, if (!pkg_subpath.length()) { return PackageMainResolve(env, pjson_url, *pcfg.FromJust(), base); } else { - return FinalizeResolution(env, URL(pkg_subpath, pjson_url), base); + if (pcfg.FromJust()->has_exports == HasExports::Yes) { + return PackageExportsResolve(env, pjson_url, pkg_subpath, + *pcfg.FromJust(), base); + } else { + return FinalizeResolution(env, URL(pkg_subpath, pjson_url), base); + } } CHECK(false); // Cross-platform root check. diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs new file mode 100644 index 0000000000..45f1518dfd --- /dev/null +++ b/test/es-module/test-esm-exports.mjs @@ -0,0 +1,15 @@ +// Flags: --experimental-modules + +import { mustCall } from '../common/index.mjs'; +import { ok, strictEqual } from 'assert'; + +import { asdf, asdf2 } from '../fixtures/pkgexports.mjs'; +import { loadMissing } from '../fixtures/pkgexports-missing.mjs'; + +strictEqual(asdf, 'asdf'); +strictEqual(asdf2, 'asdf'); + +loadMissing().catch(mustCall((err) => { + ok(err.message.toString().startsWith('Package exports')); + ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); +})); diff --git a/test/fixtures/node_modules/pkgexports/asdf.js b/test/fixtures/node_modules/pkgexports/asdf.js new file mode 100644 index 0000000000..683f2d8ba6 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/asdf.js @@ -0,0 +1 @@ +module.exports = 'asdf'; diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json new file mode 100644 index 0000000000..9bdd9b6ebb --- /dev/null +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -0,0 +1,6 @@ +{ + "exports": { + "./asdf": "./asdf.js", + "./sub/": "./" + } +} diff --git a/test/fixtures/pkgexports-missing.mjs b/test/fixtures/pkgexports-missing.mjs new file mode 100644 index 0000000000..79758545ea --- /dev/null +++ b/test/fixtures/pkgexports-missing.mjs @@ -0,0 +1,3 @@ +export function loadMissing () { + return import('pkgexports/missing'); +} \ No newline at end of file diff --git a/test/fixtures/pkgexports.mjs b/test/fixtures/pkgexports.mjs new file mode 100644 index 0000000000..6166207f70 --- /dev/null +++ b/test/fixtures/pkgexports.mjs @@ -0,0 +1,2 @@ +export { default as asdf } from 'pkgexports/asdf'; +export { default as asdf2 } from 'pkgexports/sub/asdf.js'; \ No newline at end of file From 3c70334f542b6777bb1d89aa6a869335a330e2c5 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Tue, 2 Jul 2019 11:59:46 -0400 Subject: [PATCH 2/5] add documentation --- doc/api/esm.md | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/doc/api/esm.md b/doc/api/esm.md index c4edc52357..ee71cf61d4 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -216,6 +216,59 @@ a package would be accessible like `require('pkg')` and `import module entry point and legacy users could be informed of the CommonJS entry point path, e.g. `require('pkg/commonjs')`. +## Package Exports + +By default, all subpaths from a package can be imported (`import 'pkg/x.js'`). +Custom subpath aliasing and encapsulation can be provided through the +`"exports"` field. + + +```js +// ./node_modules/es-module-package/package.json +{ + "exports": { + "./submodule": "./src/submodule.js" + } +} +``` + +```js +import submodule from 'es-module-package/submodule'; +// Loads ./node_modules/es-module-package/src/submodule.js +``` + +In addition to defining an alias, subpaths not defined by `"exports"` will +throw when an attempt is made to import them: + +```js +import submodule from 'es-module-package/private-module.js'; +// Throws - Package exports error +``` + +> Note: this is not a strong encapsulation as any private modules can still be +> loaded by absolute paths. + +Folders can also be mapped with package exports as well: + + +```js +// ./node_modules/es-module-package/package.json +{ + "exports": { + "./features/": "./src/features/" + } +} +``` + +```js +import feature from 'es-module-package/features/x.js'; +// Loads ./node_modules/es-module-package/src/features/x.js +``` + +If a package has no exports, setting `"exports": false` can be used instead of +`"exports": {}` to indicate the package does not intent for submodules to be +exposed. + ## import Specifiers ### Terminology From c86a017765eb29fd8e6a3dfb83031a9144e0e6ee Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Thu, 4 Jul 2019 09:32:25 -0700 Subject: [PATCH 3/5] test: Add missing newline to test files --- test/fixtures/pkgexports-missing.mjs | 2 +- test/fixtures/pkgexports.mjs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/fixtures/pkgexports-missing.mjs b/test/fixtures/pkgexports-missing.mjs index 79758545ea..bbf123e330 100644 --- a/test/fixtures/pkgexports-missing.mjs +++ b/test/fixtures/pkgexports-missing.mjs @@ -1,3 +1,3 @@ export function loadMissing () { return import('pkgexports/missing'); -} \ No newline at end of file +} diff --git a/test/fixtures/pkgexports.mjs b/test/fixtures/pkgexports.mjs index 6166207f70..4d82ba0560 100644 --- a/test/fixtures/pkgexports.mjs +++ b/test/fixtures/pkgexports.mjs @@ -1,2 +1,2 @@ export { default as asdf } from 'pkgexports/asdf'; -export { default as asdf2 } from 'pkgexports/sub/asdf.js'; \ No newline at end of file +export { default as asdf2 } from 'pkgexports/sub/asdf.js'; From 29df8a0dad6bb5b28b5d6eb7ecb68f85f86a6404 Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Thu, 4 Jul 2019 09:50:11 -0700 Subject: [PATCH 4/5] esm: Treat all non-nullish values the same --- doc/api/esm.md | 2 ++ src/module_wrap.cc | 3 +-- test/es-module/test-esm-exports.mjs | 7 ++++++- test/fixtures/node_modules/pkgexports-number/hidden.js | 1 + test/fixtures/node_modules/pkgexports-number/package.json | 3 +++ test/fixtures/pkgexports-missing.mjs | 6 +++++- 6 files changed, 18 insertions(+), 4 deletions(-) create mode 100644 test/fixtures/node_modules/pkgexports-number/hidden.js create mode 100644 test/fixtures/node_modules/pkgexports-number/package.json diff --git a/doc/api/esm.md b/doc/api/esm.md index ee71cf61d4..05d01f7390 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -268,6 +268,8 @@ import feature from 'es-module-package/features/x.js'; If a package has no exports, setting `"exports": false` can be used instead of `"exports": {}` to indicate the package does not intent for submodules to be exposed. +This is just a convention that works because `false`, just like `{}`, has no +iterable own properties. ## import Specifiers diff --git a/src/module_wrap.cc b/src/module_wrap.cc index b7c0e8bdcf..0574f896df 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -615,8 +615,7 @@ Maybe GetPackageConfig(Environment* env, Persistent exports; if (pkg_json->Get(env->context(), env->exports_string()).ToLocal(&exports_v) && - (exports_v->IsObject() || - (exports_v->IsBoolean() && exports_v->IsFalse()))) { + !exports_v->IsNullOrUndefined()) { exports.Reset(env->isolate(), exports_v); auto entry = env->package_json_cache.emplace(path, diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index 45f1518dfd..fc3c07d8bc 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -4,7 +4,7 @@ import { mustCall } from '../common/index.mjs'; import { ok, strictEqual } from 'assert'; import { asdf, asdf2 } from '../fixtures/pkgexports.mjs'; -import { loadMissing } from '../fixtures/pkgexports-missing.mjs'; +import { loadMissing, loadFromNumber } from '../fixtures/pkgexports-missing.mjs'; strictEqual(asdf, 'asdf'); strictEqual(asdf2, 'asdf'); @@ -13,3 +13,8 @@ loadMissing().catch(mustCall((err) => { ok(err.message.toString().startsWith('Package exports')); ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); })); + +loadFromNumber().catch(mustCall((err) => { + ok(err.message.toString().startsWith('Package exports')); + ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); +})); diff --git a/test/fixtures/node_modules/pkgexports-number/hidden.js b/test/fixtures/node_modules/pkgexports-number/hidden.js new file mode 100644 index 0000000000..c04e6ee618 --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-number/hidden.js @@ -0,0 +1 @@ +module.exports = 'not-part-of-api'; diff --git a/test/fixtures/node_modules/pkgexports-number/package.json b/test/fixtures/node_modules/pkgexports-number/package.json new file mode 100644 index 0000000000..315f39a66e --- /dev/null +++ b/test/fixtures/node_modules/pkgexports-number/package.json @@ -0,0 +1,3 @@ +{ + "exports": 42 +} diff --git a/test/fixtures/pkgexports-missing.mjs b/test/fixtures/pkgexports-missing.mjs index bbf123e330..ab7705a150 100644 --- a/test/fixtures/pkgexports-missing.mjs +++ b/test/fixtures/pkgexports-missing.mjs @@ -1,3 +1,7 @@ -export function loadMissing () { +export function loadMissing() { return import('pkgexports/missing'); } + +export function loadFromNumber() { + return import('pkgexports-number/hidden.js'); +} From fd6a1076f75563d2271066f98b0bf3dbf23310f0 Mon Sep 17 00:00:00 2001 From: Jan Krems Date: Thu, 4 Jul 2019 10:07:13 -0700 Subject: [PATCH 5/5] esm: Remove support for dot main --- src/module_wrap.cc | 14 -------------- test/es-module/test-esm-exports.mjs | 10 +++++++++- test/fixtures/node_modules/pkgexports/package.json | 1 + test/fixtures/pkgexports-missing.mjs | 4 ++++ 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 0574f896df..037d691f3a 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -776,20 +776,6 @@ Maybe PackageMainResolve(Environment* env, const PackageConfig& pcfg, const URL& base) { if (pcfg.exists == Exists::Yes) { - Local exports = pcfg.exports.Get(env->isolate()); - if (pcfg.has_exports == HasExports::Yes && exports->IsObject()) { - Local exports_obj = exports.As(); - Local dot_string = String::NewFromUtf8(env->isolate(), ".", - v8::NewStringType::kNormal).ToLocalChecked(); - auto dot_main = - exports_obj->Get(env->context(), dot_string).ToLocalChecked(); - if (dot_main->IsString()) { - Utf8Value main_utf8(env->isolate(), dot_main.As()); - std::string main(*main_utf8, main_utf8.length()); - URL main_url("./" + main, pjson_url); - return FinalizeResolution(env, main_url, base); - } - } if (pcfg.has_main == HasMain::Yes) { URL resolved(pcfg.main, pjson_url); const std::string& path = resolved.ToFilePath(); diff --git a/test/es-module/test-esm-exports.mjs b/test/es-module/test-esm-exports.mjs index fc3c07d8bc..5852e7e7da 100644 --- a/test/es-module/test-esm-exports.mjs +++ b/test/es-module/test-esm-exports.mjs @@ -4,7 +4,11 @@ import { mustCall } from '../common/index.mjs'; import { ok, strictEqual } from 'assert'; import { asdf, asdf2 } from '../fixtures/pkgexports.mjs'; -import { loadMissing, loadFromNumber } from '../fixtures/pkgexports-missing.mjs'; +import { + loadMissing, + loadFromNumber, + loadDot, +} from '../fixtures/pkgexports-missing.mjs'; strictEqual(asdf, 'asdf'); strictEqual(asdf2, 'asdf'); @@ -18,3 +22,7 @@ loadFromNumber().catch(mustCall((err) => { ok(err.message.toString().startsWith('Package exports')); ok(err.message.toString().indexOf('do not define a \'./missing\' subpath')); })); + +loadDot().catch(mustCall((err) => { + ok(err.message.toString().startsWith('Cannot find main entry point')); +})); diff --git a/test/fixtures/node_modules/pkgexports/package.json b/test/fixtures/node_modules/pkgexports/package.json index 9bdd9b6ebb..51c596ed86 100644 --- a/test/fixtures/node_modules/pkgexports/package.json +++ b/test/fixtures/node_modules/pkgexports/package.json @@ -1,5 +1,6 @@ { "exports": { + ".": "./asdf.js", "./asdf": "./asdf.js", "./sub/": "./" } diff --git a/test/fixtures/pkgexports-missing.mjs b/test/fixtures/pkgexports-missing.mjs index ab7705a150..7d1d5b2e82 100644 --- a/test/fixtures/pkgexports-missing.mjs +++ b/test/fixtures/pkgexports-missing.mjs @@ -5,3 +5,7 @@ export function loadMissing() { export function loadFromNumber() { return import('pkgexports-number/hidden.js'); } + +export function loadDot() { + return import('pkgexports'); +}