search

sass / dart-sass

The reference implementation of Sass, written in Dart.
https://sass-lang.com/dart-sass
MIT License
3.97k stars 360 forks source link

Node package importer requires an entryPointDirectory in ESM #2178

Closed jamesnw closed 8 months ago

jamesnw commented 8 months ago

See https://github.com/sass/sass/issues/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 https://github.com/sass/dart-sass/pull/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 commented 8 months ago

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 https://github.com/nodejs/node/issues/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, https://github.com/nodejs/node/issues/49440#issuecomment-1703467948 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.)