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

esm: allow configuring default package type #32394

Closed
wants to merge 4 commits into from
Closed
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 configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,12 @@
default=False,
help='node will load builtin modules from disk instead of from binary')

parser.add_option('--default-package-type',
action='store',
dest='node_default_package_type',
default=False,
help='node will treat files outside of package boundaries as this type')

# Create compile_commands.json in out/Debug and out/Release.
parser.add_option('-C',
action='store_true',
Expand Down Expand Up @@ -1185,6 +1191,11 @@ def configure_node(o):
print('Warning! Loading builtin modules from disk is for development')
o['variables']['node_builtin_modules_path'] = options.node_builtin_modules_path

if options.node_default_package_type:
if not ( options.node_default_package_type in ['module', 'commonjs'] ):
raise Exception('--default-package-type must be either "module" or "commonjs"');
o['variables']['node_default_package_type'] = options.node_default_package_type

def configure_napi(output):
version = getnapibuildversion.get_napi_version()
output['variables']['napi_build_version'] = version
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/modules/esm/get_format.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const experimentalWasmModules = getOptionValue('--experimental-wasm-modules');
const { getPackageType } = require('internal/modules/esm/resolve');
const { URL, fileURLToPath } = require('internal/url');
const { ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes;
const { defaultPackageType } = internalBinding('config');

const extensionFormatMap = {
'__proto__': null,
Expand Down Expand Up @@ -51,7 +52,8 @@ function defaultGetFormat(url, context, defaultGetFormatUnused) {
const ext = extname(parsed.pathname);
let format;
if (ext === '.js') {
format = getPackageType(parsed.href) === 'module' ? 'module' : 'commonjs';
const type = getPackageType(parsed.href);
format = type !== 'none' ? type : defaultPackageType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This defaultPackageType implementation can move into the getPackageType implementation itself. Alternatively getPackageType can return eg undefined to indicate the default should be used.

I just want to avoid explicitly supporting a new "type": "none" value in the public API.

} else {
format = extensionFormatMap[ext];
}
Expand Down
8 changes: 6 additions & 2 deletions lib/internal/modules/run_main.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const CJSLoader = require('internal/modules/cjs/loader');
const { Module, toRealPath, readPackageScope } = CJSLoader;
const { getOptionValue } = require('internal/options');
const path = require('path');
const { defaultPackageType } = internalBinding('config');

function resolveMainPath(main) {
// Note extension resolution for the main entry point can be deprecated in a
Expand Down Expand Up @@ -33,8 +34,11 @@ function shouldUseESMLoader(mainPath) {
return true;
if (!mainPath || mainPath.endsWith('.cjs'))
return false;
const pkg = readPackageScope(mainPath);
return pkg && pkg.data.type === 'module';
const pkgType = readPackageScope(mainPath)?.data?.type ?? 'none';
return pkgType === 'module' || (
pkgType === 'none' &&
defaultPackageType === 'module'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make the default "commonjs" instead of "none" since that is what we've defined to be the default.

}

function runMainESM(mainPath) {
Expand Down
4 changes: 4 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
'node_lib_target_name%': 'libnode',
'node_intermediate_lib_type%': 'static_library',
'node_builtin_modules_path%': '',
'node_default_package_type%': '',
'library_files': [
'lib/internal/bootstrap/environment.js',
'lib/internal/bootstrap/loaders.js',
Expand Down Expand Up @@ -748,6 +749,9 @@
[ 'node_builtin_modules_path!=""', {
'defines': [ 'NODE_BUILTIN_MODULES_PATH="<(node_builtin_modules_path)"' ]
}],
[ 'node_default_package_type!=""', {
'defines': [ 'NODE_DEFAULT_PACKAGE_TYPE="<(node_default_package_type)"' ]
}],
[ 'node_shared=="true"', {
'sources': [
'src/node_snapshot_stub.cc',
Expand Down
16 changes: 16 additions & 0 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ using v8::Isolate;
using v8::Local;
using v8::Number;
using v8::Object;
using v8::String;
bmeck marked this conversation as resolved.
Show resolved Hide resolved
using v8::Value;

// The config binding is used to provide an internal view of compile time
Expand Down Expand Up @@ -85,6 +86,21 @@ static void Initialize(Local<Object> target,
READONLY_TRUE_PROPERTY(target, "hasDtrace");
#endif

#if defined NODE_DEFAULT_PACKAGE_TYPE
READONLY_PROPERTY(target,
"defaultPackageType",
String::NewFromOneByte(isolate,
reinterpret_cast<const unsigned char *>(
NODE_DEFAULT_PACKAGE_TYPE))
.ToLocalChecked());
bmeck marked this conversation as resolved.
Show resolved Hide resolved
#else
READONLY_PROPERTY(target,
"defaultPackageType",
String::NewFromOneByte(isolate,
reinterpret_cast<const unsigned char *>("commonjs"))
.ToLocalChecked());
bmeck marked this conversation as resolved.
Show resolved Hide resolved
#endif

READONLY_PROPERTY(target, "hasCachedBuiltins",
v8::Boolean::New(isolate, native_module::has_code_cache));
} // InitConfig
Expand Down