Skip to content

module: add --package flag for implicit config testing #38028

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

module: add --package flag for implicit config testing #38028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 24 additions & 1 deletion lib/internal/bootstrap/pre_execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ const {
const { reconnectZeroFillToggle } = require('internal/buffer');

const { Buffer } = require('buffer');
const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes;
const {
ERR_MANIFEST_ASSERT_INTEGRITY,
ERR_PACKAGE_NOT_ALLOWED
} = require('internal/errors').codes;
const assert = require('internal/assert');

function prepareMainThreadExecution(expandArgv1 = false) {
Expand Down Expand Up @@ -97,14 +100,34 @@ function patchProcessObject(expandArgv1) {
});
process.argv[0] = process.execPath;

let base;
if (expandArgv1 && process.argv[1] &&
!StringPrototypeStartsWith(process.argv[1], '-')) {
// Expand process.argv[1] into a full path.
const path = require('path');
try {
process.argv[1] = path.resolve(process.argv[1]);
base = path.dirname(process.argv[1]);
} catch {}
}
const implicitPackageJSONPath = getOptionValue('--package');
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree with @DerekNonGeneric probably package-json-file or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer not package-json since the target could be named something like module-package.test we could go for package-file, does that seem fine?

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think --package-config would be preferable since that is what it is called elsewhere.

node/lib/internal/errors.js

Line 1183 in 3ef9562

E('ERR_INVALID_PACKAGE_CONFIG', (path, base, message) => {

Copy link
Contributor

Choose a reason for hiding this comment

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

How about --package-path and then also making it optional to include the package.json part?

Copy link
Member Author

Choose a reason for hiding this comment

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

@guybedford I'm not liking the excluding of the package.json since it is part of the path and we don't do searching generally these days. Additionally if we are fully explicit I wonder if we could do a inline-src like @aduh95 mentioned in #37848 (comment) for other stuff. If we do searching having it map directly to a URL would be weirder.

if (implicitPackageJSONPath) {
const cwd = process.cwd();
if (!base) {
base = cwd;
}
const path = require('path');
const { read, clobber } = require('internal/modules/package_json_reader');
const cwdPackage = read(path.join(cwd, 'package.json'));
const entryPackage = read(path.join(base, 'package.json'));
if (
typeof cwdPackage.string === 'string' ||
typeof entryPackage.string === 'string'
) {
throw ERR_PACKAGE_NOT_ALLOWED();
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a reasonable tradeoff for now.

}
clobber(path.join(base, 'package.json'), implicitPackageJSONPath);
}

// TODO(joyeecheung): most of these should be deprecated and removed,
// except some that we need to be able to mutate during run time.
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,8 @@ E('ERR_OUT_OF_RANGE',
msg += ` It must be ${range}. Received ${received}`;
return msg;
}, RangeError);
E('ERR_PACKAGE_NOT_ALLOWED', '--package can only be used when there is not an' +
' existing package.json file', Error);
E('ERR_PACKAGE_IMPORT_NOT_DEFINED', (specifier, packagePath, base) => {
return `Package import specifier "${specifier}" is not defined${packagePath ?
` in package ${packagePath}package.json` : ''} imported from ${base}`;
Expand Down
12 changes: 11 additions & 1 deletion lib/internal/modules/package_json_reader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,14 @@ function read(jsonPath) {
return result;
}

module.exports = { read };
/**
*
* @param {string} jsonPath
* @param {string} srcPath
*/
function clobber(jsonPath, srcPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about just making this set, and then providing the direct JSON to set into the cache? Since read is called before the usage of clobber anyway. This then also opens the door to possible inline package configuration options.

Copy link
Member Author

Choose a reason for hiding this comment

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

inline package configuration??

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! (eg node app.js --pcfg.type=module as a natural extension)

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, i was imagining just allowing a source text for the file since manually configuring multiple of them would be noise for the args parser

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I wouldn't want to speculate, suggestion was as much about read + clobber = read + read + write -> read + write being the simplification.

const value = cache.has(srcPath) ? cache.get(srcPath) : read(srcPath);
cache.set(jsonPath, value);
}

module.exports = { clobber, read };
4 changes: 4 additions & 0 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
"silence all process warnings",
&EnvironmentOptions::no_warnings,
kAllowedInEnvironment);
AddOption("--package",
"set file to act as package for entry point",
&EnvironmentOptions::implicit_package,
kAllowedInEnvironment);
AddOption("--force-context-aware",
"disable loading non-context-aware addons",
&EnvironmentOptions::force_context_aware,
Expand Down
2 changes: 2 additions & 0 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ class EnvironmentOptions : public Options {
bool tls_max_v1_3 = false;
std::string tls_keylog;

std::string implicit_package;

std::vector<std::string> preload_modules;

std::vector<std::string> user_argv;
Expand Down