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

modules: add import.meta.__dirname and import.meta.__filename #39147

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 24, 2021

To help ease the transition from existing CommonJS code to
ESM, adds convenience properties to import.meta that are
explicitly marked "Legacy" in the documentation.

This may or may not be a good idea, but I've run into this
enough times that I figured I'd at least see. If folks don't
think this is a good idea, then I'm fine with it not landing.

const {
  __filename,
  __dirname,
} = import.meta;

They are only available when file:// modules are used.

To help ease the transition from existing CommonJS code to
ESM, adds convenience properties to `import.meta` that are
explicitly marked "Legacy" in the documentation.

Signed-off-by: James M Snell <jasnell@gmail.com>
@github-actions github-actions bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jun 24, 2021
@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2021

If this did happen to land, I'd suggest it would be a good idea to contribute an eslint rule that flags uses and recommends the equivalent import.meta.url uses instead.

@@ -5,6 +5,7 @@ const {
ArrayPrototypeMap,
Boolean,
JSONParse,
ObjectDefineProperties,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ObjectDefineProperties,

Unused primordial.

@devsnek
Copy link
Member

devsnek commented Jun 24, 2021

If these are legacy, that implies a deprecation/transition period right? Would migrating a file from cjs to esm not already be that transition? People will just use these and then we'll have to support them forever even though they're bad.

@jasnell
Copy link
Member Author

jasnell commented Jun 24, 2021

To be clear, legacy status does not imply eventual deprecation.

`__dirname` pseudo-global. Specifically, it returns the result of converting
the `import.meta.url` into a file path then returning the directory portion.

The property is only available when `import.meta.url` specifies a `file://`
Copy link
Member

Choose a reason for hiding this comment

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

the check in the impl only uses file: should you be using // in the impl or remove that bit from here to just be file:? generally when talking about scheme we just have scheme: (E.G. https://nodejs.org/dist/latest-v14.x/docs/api/esm.html#esm_file_urls )

Comment on lines +135 to +136
meta.__filename = internalURLModule.fileURLToPath(new URL(url));
meta.__dirname = internalPathModule.dirname(meta.__filename);
Copy link
Member

Choose a reason for hiding this comment

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

can this be done with getters / lazily?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not, there's a test that checks that import.meta properties are writable

Copy link
Member

Choose a reason for hiding this comment

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

seems fine

@targos
Copy link
Member

targos commented Jun 25, 2021

As an alternative or in parallel, I think we should also improve the documentation at https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_no_filename_or_dirname or https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_import_meta_url, by giving examples of how common use cases can be done with import.meta.url

@bricss
Copy link

bricss commented Jun 28, 2021

I was hoping that one day we will get rid of CJS legacy aka __filename, __dirname, but yet it's coming back 🥶

@bmeck
Copy link
Member

bmeck commented Jun 28, 2021

Do we have a clear problem with including these? It won't cause a static unrecoverable error if these values are used.

My main concern is this just adds 1 more code migration, though it is incremental. Adding them to import.meta still requires that code be updated so that they can be used. It also still means that examples are not shared between CJS and ESM:

// __dirname is a ReferenceError here
fs.readFile(import.meta.__dirname);
// import.meta is a SyntaxError here
fs.readFile(__dirname);

I think an approach that allows both CJS and ESM to share the code would be somewhat preferable to this, but I don't have any major concerns w/ these being added except that they would be wasted compute for any module not using them and using import.meta.url (and vice versa).

@jasnell
Copy link
Member Author

jasnell commented Jun 29, 2021

given the controversy here and given that I'm not 100% bought in to this idea either, I'm going to go ahead and close this :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants