esm: add import.meta.require

[marco-ippolito]

History:

• Pull request opened for import.meta.require on core modules#130
• Pull request opened for import.meta.require on core modules#130

Consider the following cjs snippet:

function getJSON(path){
  const file = require(path);
}

The equivalent in esm would be something like (more or less):

import { createRequire } from "node:module"
function getJSON(path){
  const require = createRequire(import.meta.url);
  const file = require(path);
}
// or asyncronously
async function getJSON(path){
  const file = await import(path, { with: { type: 'json' } });
}

We could improve the dx with the new import.meta.require:

function getJSON(path){
  const file = import.meta.require(path);
}

Now that we have cjs/esm interoperability we can create import.meta.require to syncronously require modules in ESM.

It's just an alias for:

import { createRequire } from 'node:module';

const require = createRequire(import.meta.url);

const foo = require('foo.js');

WinterCG PR: wintercg/import-meta-registry#9

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[JakobJingleheimer]

Did the discussion settle on a decision around the perf implications?

[marco-ippolito]

Did the discussion settle on a decision around the perf implications?

Would be great to have some time at the collab-summit to discuss

[targos]

Is there still demand for it? I'm not against it but I don't expect many people to need it when you can usually just import the common js modules.

[ljharb]

The primary use cases I can see for this is importing JSON files, so you don't have to write all the with boilerplate; and, to leverage the CJS resolution algorithm for ergonomic reasons.

[JakobJingleheimer]

🙌 LGTM (assuming it's needed and the potential perf concern is 👍)

[guybedford]

This seems a little weakly motivated in the PR description - it would help a lot to get more background on the specific use cases this is seeking to solve.

[marco-ippolito]

This needs to go through WinterCG before landing.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (58a7b00) to head (44652ff).
Report is 42 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #55730    +/-   ##
========================================
  Coverage   88.40%   88.40%            
========================================
  Files         654      654            
  Lines      187655   187773   +118     
  Branches    36111    36131    +20     
========================================
+ Hits       165889   165997   +108     
- Misses      14996    15008    +12     
+ Partials     6770     6768     -2     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/initialize_import_meta.js	100.00% <100.00%> (ø)

... and 39 files with indirect coverage changes

[jasnell]

This needs to go through WinterCG before landing.

For wintercg, the most that needs to happen is a PR adding the new thing to https://github.com/wintercg/import-meta-registry

[marco-ippolito]

WinterCG PR: wintercg/import-meta-registry#9
According to winterCG it needs to wait for a week before landing.

[ShogunPanda]

LGTM!

[guybedford]

I've posted my arguments against this in the WinterCG issue here - wintercg/import-meta-registry#9 (comment). If Node.js does not want to discuss this in a WinterCG context and would rather make it a project issue I'm happy to bring discussion back here too.

[aduh95]

I would much prefer to see a different API, ideally one that could be supported by other runtimes, to address that specific use-case. Adding a request for changes to make sure this doesn't land before the cross runtime solution have been ruled out.

[marco-ippolito]

I would much prefer to see a different API, ideally one that could be supported by other runtimes, to address that specific use-case. Adding a request for changes to make sure this doesn't land before the cross runtime solution have been ruled out.

The feedback is not very useful honestly. If you propose changes to the API please suggest what kind of API you would like to see.

Note: the PR already has the "blocked" label, so I don't see the reason to request changes

I also dont agree with the changes proposed, all other import.meta apis are writable, so it makes sense to rely on the setter.

[aduh95]

all other import.meta apis are writable, so it makes sense to rely on the setter

I really don't see why the former would imply the latter 😅

I also dont agree with the changes proposed

The setOwnProperty helper is there precisely so we don't have to write the same ObjectDefineProperty is multiple places, and arguably makes the code more readable, what's not to love about that?

[marco-ippolito]

See wintercg/import-meta-registry#9 (comment)

[guybedford]

I've posted https://github.com/guybedford/proposal-import-sync to follow-up exploration in this space, and will add it to the agenda for the TC39 modules group meeting on Thursday. If anyone is interested in discussing this you are welcome to attend, the event is available on the public TC39 calendar here - https://calendar.google.com/calendar/u/0/event?eid=c2luNm9jcm4wbTJyNXNxY2JkbGZtdmdwZWNfMjAyNDExMTRUMTYwMDAwWiAzN2EyMTA3ZGZmMTUxOTNiNDJmZmZhMDkxYmM5OTkxNjU2OTVkNDNiN2U0ZjQzYjY1ZWFiMDkzZGEyNzU3YTNhQGc.

[joyeecheung]

By the way I don't think import.sync is functionally a proper replacement for import.meta.require in that default exports would still be treated differently and there is no way to customize what it returns directly. I was just looking at https://tailwindcss.com/docs/configuration for some bug in #56140 and started to think, whether tailwind can just load ESM .js config files now that we have require(esm), but then you'd need to properly replace the API

module.exports = {
  plugins: [
    require('@tailwindcss/forms'),
  ],
}

Now if you do this

const config = {
  plugins: [
    ????('@tailwindcss/forms'),
  ],
}

export default config;
export { config as "module.exports" };

The ???? still has to be require(). If the plugins are shipped as CJS, import.sync() is going to return { default: plugin, 'module.exports': plugin } instead of plugin. Or if the plugins are shipped as ESM (since we support require(esm) now), import.sync() is still going to return { default: plugin } instead of plugin, so in neither case is it going to be a proper replacement for require(), unless there is a way to make import.sync() support full customization of what it returns, much like the "module.exports" marker we use to support what require(esm) returns.

The example may be easier to workaround if users have control over the config and can just unwrap it, but compatibility issues can still arise from fragmented versions or if the return values get encapsulated and passed around (imagine the API just takes specifiers instead of giving control to users how it should be loaded), and this return value mismatch can happen to many other use cases. In that vein having a non-functional replacement just means it'll encourage people to do const require = createRequire(import.meta.url) to workaround its caveat, which compared to import.meta.require is likely to make the dependency harder to detect for tree shaking/bundling, instead of eaiser.

[marco-ippolito]

I think this is still worth pursuing (in a different form), I will reiterate on this once I'm back from vacation