node icon indicating copy to clipboard operation
node copied to clipboard

process: add process.getBuiltinModule(id)

Open joyeecheung opened this issue 9 months ago • 20 comments

process: add process.getBuiltinModule(id)

process.getBuiltinModule(id) provides a way to load built-in modules in a globally available function. ES Modules that need to support other environments can use it to conditionally load a Node.js built-in when it is run in Node.js, without having to deal with the resolution error that can be thrown by import in a non-Node.js environment or having to use dynamic import() which either turns the module into an asynchronous module, or turns a synchronous API into an asynchronous one.

if (globalThis.process.getBuiltinModule) {
  // Run in Node.js, use the Node.js fs module.
  const fs = globalThis.process.getBuiltinModule('fs');
  // If `require()` is needed to load user-modules, use
  // createRequire()
  const module = globalThis.process.getBuiltinModule('module');
  const require = module.createRequire(import.meta.url);
  const foo = require('foo');
}

If id specifies a built-in module available in the current Node.js process, process.getBuiltinModule(id) method returns the corresponding built-in module. If id does not correspond to any built-in module, undefined is returned.

process.getBuiltinModule(id) accept built-in module IDs that are recognized by module.isBuiltin(id). Some built-in modules must be loaded with the node: prefix.

The built-in modules returned by process.getBuiltinModule(id) are always the original modules - that is, it's not affected by require.cache.

Fixes: https://github.com/nodejs/node/issues/52599

joyeecheung avatar Apr 30 '24 16:04 joyeecheung

Review requested:

  • [ ] @nodejs/loaders
  • [ ] @nodejs/startup

nodejs-github-bot avatar Apr 30 '24 16:04 nodejs-github-bot

This is obviously exactly what I asked for (😄) and gets it "done" the quickest, but I'm curious if there are any other opinions about this in general.

Is it possible that import.now or "weak/optional" imports come to fruition, such that those are more usable and processed by downstream tools? I'd hate to accidentally open the door to "breaking" node polyfills (node-polyfill-webpack-plugin, vite-plugin-node-polyfills, etc) and so on, just because it's not require or import, but some other expression.

jakebailey avatar May 01 '24 02:05 jakebailey

I'd hate to accidentally open the door to "breaking" node polyfills (node-polyfill-webpack-plugin, vite-plugin-node-polyfills

Not quite sure if I am following this but I think process.getBuiltin(id) should be polyfill-able by these kind of bundler plugins - just patch the its polyfilled process object to load its own polyfilled libraries by id? (In general plugins like this already polyfill the process object). For code that uses the polyfill/gets transformed to use the polyfills, if the polyfill doesn't support this yet, using this would not be too different than "using a new API that the polyfill doesn't support yet" which happens all the time.

joyeecheung avatar May 01 '24 02:05 joyeecheung

Yeah, I guess it's no different than detecting calls to require and error/warn on ones which cannot be statically determined. It's just not quite as injectable as require.

Just poking holes in my own idea.

jakebailey avatar May 01 '24 02:05 jakebailey

We can also suggest in the documentation that users should only consider using this for code paths that can only be run in Node.js (the code example is basically already suggesting that but we can make it clearer). If they want the code depending on the built-ins to be polyfill-able in the browser, they should still use require or import.

joyeecheung avatar May 01 '24 02:05 joyeecheung

There is a TC39 proposal to get original built-in functions, through a new getIntrinsic global (https://github.com/tc39/proposal-get-intrinsic). getBuiltin is similar enough that could cause confusion — what do you think about something like process.getBuiltinModule to be more clear about what it does?

nicolo-ribaudo avatar May 01 '24 07:05 nicolo-ribaudo

Just to prove this out to its full extent (and to procrastinate other work I didn't want to do), I polyfilled this API using --require and was able to get TypeScript's CLI, API, and test suite all working: https://github.com/microsoft/TypeScript/pull/58419

jakebailey avatar May 02 '24 23:05 jakebailey

Updated the PR a bit:

  • Changed it to process.getBuiltinModule() as requested in https://github.com/nodejs/node/pull/52762#issuecomment-2088093800
  • Return undefined for IDs without corresponding modules as requested in https://github.com/nodejs/node/pull/52762#discussion_r1590009551
  • Accept IDs that are accepted by module.isBuiltin and reject otherwise (so this now throws on getBuiltinModule('test') etc) as requested in https://github.com/nodejs/node/pull/52762#discussion_r1585606673

joyeecheung avatar May 07 '24 02:05 joyeecheung

CI: https://ci.nodejs.org/job/node-test-pull-request/59001/

nodejs-github-bot avatar May 07 '24 02:05 nodejs-github-bot

Changed it to process.getBuiltinModule() as requested in process: add process.getBuiltinModule(id)

Now it mismatches isBuiltin. Personally I don't see the confusion between getIntrinsic and getBuiltin; I think getBuiltin is fine, and there's more potential for confusion with this not corresponding with isBuiltin.

Alternatively we can add isBuiltinModule as an alias for isBuiltin.

GeoffreyBooth avatar May 07 '24 03:05 GeoffreyBooth

Now it mismatches isBuiltin. Personally I don't see the confusion between getIntrinsic and getBuiltin; I think getBuiltin is fine, and there's more potential for confusion with this not corresponding with isBuiltin.

Alternatively we can add isBuiltinModule as an alias for isBuiltin.

isBuiltin is scoped in node:module. It is not a function available from the global scope, like global.process.getBuiltinModule.

legendecas avatar May 07 '24 16:05 legendecas

isBuiltin is scoped in node:module. It is not a function available from the global scope, like global.process.getBuiltinModule.

Fair enough. I still think we should create an alias on node:module isBuiltinModule, but that can come later.

GeoffreyBooth avatar May 07 '24 17:05 GeoffreyBooth

CI: https://ci.nodejs.org/job/node-test-pull-request/59265/

nodejs-github-bot avatar May 17 '24 10:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59310/

nodejs-github-bot avatar May 20 '24 07:05 nodejs-github-bot

I'm not a core collaborator, so this is only a suggestion: Shouldn't this become a no-op with --experimental-permission?

I don't think it's relevant for the permission model as that guards things at the native layer when resources are actually accessed. The removed policy feature did guard access to builtins at the module loader level, but that's..removed (in general guarding things at the module loader level isn't super useful anyway because how widely monkey-patching of the module loader is used). But cc @nodejs/security-wg to be sure.

joyeecheung avatar May 20 '24 16:05 joyeecheung

CI: https://ci.nodejs.org/job/node-test-pull-request/59319/

nodejs-github-bot avatar May 20 '24 16:05 nodejs-github-bot

I'm not a core collaborator, so this is only a suggestion: Shouldn't this become a no-op with --experimental-permission?

I don't think it's relevant for the permission model as that guards things at the native layer when resources are actually accessed. The removed policy feature did guard access to builtins at the module loader level, but that's..removed (in general guarding things at the module loader level isn't super useful anyway because how widely monkey-patching of the module loader is used). But cc @nodejs/security-wg to be sure.

Thanks, I was thinking this was like process.binding, my bad, sorry

redyetidev avatar May 20 '24 17:05 redyetidev

CI: https://ci.nodejs.org/job/node-test-pull-request/59333/

nodejs-github-bot avatar May 21 '24 15:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59345/

nodejs-github-bot avatar May 22 '24 11:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59355/

nodejs-github-bot avatar May 22 '24 15:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59384/

nodejs-github-bot avatar May 24 '24 11:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59390/

nodejs-github-bot avatar May 24 '24 14:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59445/

nodejs-github-bot avatar May 27 '24 11:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59451/

nodejs-github-bot avatar May 27 '24 14:05 nodejs-github-bot

CI: https://ci.nodejs.org/job/node-test-pull-request/59485/

nodejs-github-bot avatar May 28 '24 12:05 nodejs-github-bot

CI is green though this needs another approval from a collaborator to land @GeoffreyBooth @JakobJingleheimer @aduh95 @VoltrexKeyva @mcollina @Qard @jasnell @targos @MoLow @legendecas @Lxxyx @RafaelGSS @marco-ippolito

joyeecheung avatar May 28 '24 17:05 joyeecheung

The https://github.com/nodejs/node/labels/notable-change label has been added by @joyeecheung.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

github-actions[bot] avatar May 28 '24 20:05 github-actions[bot]

Landed in 4796e05cc87a48d7a7069a468aada23df3af5336...1de215e285bf8afebe0b9b1908009399e3b1f9e2

nodejs-github-bot avatar May 28 '24 20:05 nodejs-github-bot

Would you be open to a PR backporting this to v20?

nicolo-ribaudo avatar Jun 18 '24 16:06 nicolo-ribaudo

This has only been in the current release (22) for a week. We will need to bake it for another week before backporting it to LTS per the policy.

joyeecheung avatar Jun 18 '24 17:06 joyeecheung