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.

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

Use process.argv[1] as a fallback for new NodePackageImporter() #275

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Use process.argv[1] as a fallback for new NodePackageImporter() #275

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
387e7fe
Use `process.argv[1]` as a fallback for `new NodePackageImporter()`
nex3 Feb 20, 2024
9b4d939
Reformat, bafflingly
nex3 Feb 21, 2024
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
5 changes: 5 additions & 0 deletions lib/src/importer-registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import {createRequire} from 'module';
import * as p from 'path';
import {URL} from 'url';
import {inspect} from 'util';
Expand All @@ -21,6 +22,10 @@
? p.resolve(entryPointDirectory)
: require.main?.filename
? p.dirname(require.main.filename)
// TODO: Find a way to use `import.meta.main` once

Check failure on line 25 in lib/src/importer-registry.ts

View workflow job for this annotation

GitHub Actions / Static analysis 

Insert `:·`
// https://github.com/nodejs/node/issues/49440 is done.
: process.argv[1]

Check failure on line 27 in lib/src/importer-registry.ts

View workflow job for this annotation

GitHub Actions / Static analysis 

Delete `·:`
Copy link
Contributor

@ntkme ntkme Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A small risk here is that process.argv is mutable and can be mutated before this is executed. Otherwise this would be a pretty good workaround for ESM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can avoid that issue here. At least if users run into it, they always have the option of passing something in explicitly to work around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand the concern correctly, this also applies to require.main.filename. We take advantage of that for testing different scenarios for CommonJS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the chance of argv getting modified would be higher as it’s pretty common to do something like arg = argv.shift() when parsing arguments.

? createRequire(process.argv[1]).resolve(process.argv[1])
: undefined;
if (!entryPointDirectory) {
throw new Error(
Expand Down
Loading