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

Node package importer requires an entryPointDirectory in ESM #2178

Closed
jamesnw opened this issue Feb 16, 2024 · 1 comment · Fixed by #2181
Closed

Node package importer requires an entryPointDirectory in ESM #2178

jamesnw opened this issue Feb 16, 2024 · 1 comment · Fixed by #2181
Assignees

Comments

@jamesnw
Copy link
Contributor

jamesnw commented Feb 16, 2024

See sass/sass#3792

In addition to #2176, we are seeing that require.main is undefined in an ESM context. This means that @JS("require.main.filename") is throwing the error `reading "filename" of undefined, which throws before the more helpful error can be thrown-

"The Node package importer cannot determine an entry point because
`require.main.filename` is not defined.
Please provide an `entryPointDirectory` to the `NodePackageImporter`."

Optional chaining is not supported by @JS syntax, but I was able to find a workaround in #2179.

The second more significant issue is that, currently, because of this, entryPointDirectory is required in an ESM context.

An equivalent to require.main.filename in ESM, import.meta.filename is added in Node 21. However, even if we waited until that was available, because the main sass.dart.js file is CommonJS, we won't have access to that. From the Node docs-
When the entry point is not a CommonJS module, require.main is undefined, and the main module is out of reach.

My proposal would be to use the current directory as a fallback when require.main.filename is not defined. It is not identical to the concept of require.main.filename, but does cover the most common case where the user is running the command from the directory that contains the package.

I confirmed that p.dirname(p.absolute('.')) as the fallback for when require.main.filename is not defined allows a NodePackageImporter() to be used without setting an entryPointDirectory in ESM, but wanted to verify that's the correct path forward before committing.

I think we had discussed the possibility of require.main not being available at one point, but had thought that the fixes made to dart_cli_pkg would address this as well.

@nex3
Copy link
Contributor

nex3 commented Feb 20, 2024

I'm still not very excited about using the working directory instead of require.main.filename, since the two are fairly meaningfully different. I'd generally prefer to suggest that users manually fall back on process.cwd(), since that's at least explicit, rather than doing it silently if we can't determine the entrypoint.

From nodejs/node#49440, it looks like we can get this information from process.argv[1], although it needs to be massaged somewhat because of the wide range of different ways an entrypoint can be specified on the command line. Specifically, nodejs/node#49440 (comment) suggests module.createRequire(process.argv[1]).resolve(process.argv[1]), which I believe should work for our use-case. (If process.argv isn't defined, we should just fall back on throwing an error.)

@nex3 nex3 self-assigned this Feb 20, 2024
nex3 added a commit to sass/sass that referenced this issue Feb 20, 2024
This also updates the spec for `NodePackageImporter` to correctly
document indicate that errors should be thrown during construction.

See sass/dart-sass#2178
nex3 added a commit to sass/sass that referenced this issue Feb 21, 2024
)

This also updates the spec for `NodePackageImporter` to correctly
document indicate that errors should be thrown during construction.

See sass/dart-sass#2178
nex3 added a commit to sass/sass that referenced this issue Mar 27, 2024
)

This also updates the spec for `NodePackageImporter` to correctly
document indicate that errors should be thrown during construction.

See sass/dart-sass#2178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants