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

Provide some mechanism to conditionally and synchronously import modules (or just builtins) from ESM #52599

Open
Tracked by #52697
jakebailey opened this issue Apr 19, 2024 · 15 comments · May be fixed by #52762
Open
Tracked by #52697
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders

Comments

@jakebailey
Copy link

jakebailey commented Apr 19, 2024

What is the problem this feature will solve?

Thanks to #51977, requiring ESM is looking to be a real possibility. As such, TypeScript is considering transitioning over to ESM in the near future (depending on when require(ESM) is unflagged, hopefully in time for TS 6.0?), as that sort of change would no longer pose compatibility problems for the vast number of downstream CJS users. This has a number of benefits, mainly that we could finally share code between tsc.js and our public API without a startup perf regression, and that we wouldn't be duplicating code in our package (thankfully only two copies remain as of TS 5.5, down from six copies in TS 4.9).

TypeScript's current public API bundle is intentionally "UMD-ish", detecting whether or not module.exports exists and using it (declaring a global otherwise), then later conditionally requiring built-in modules like fs if we believe to be running within Node. This allows us to ship one single bundle that works in Node, browsers, and bundlers alike.

However, the code that relies on conditional require is executed at the top-level as it's constructing the ts.sys object, the default "system" implementation for most of our APIs. Within CJS, this is fine, but within ESM, the only way to conditionally import something is by either:

  • Using top-level await (or doing it later asynchronously).
  • Importing createRequire from node:module.

Using top-level await breaks require(ESM), the whole reason we think we can use ESM in the first place, and TS is infamously not async and couldn't import it later. Importing node:module is a moot choice, since if we could safely import node:module, we could have just imported node:fs and so on.

So, we need some mechanism to synchronously import modules, or at least the builtins.

Given require(ESM) is now possible, it sure seems like there could be a way to safely synchronously import modules in ESM that are already require-able from CJS after #51977.

What is the feature you are proposing to solve the problem?

After discussing this in the TC39 Module Harmony meeting (with @guybedford @joyeecheung @JakobJingleheimer, others), there seemed to be a number of different paths forward:

  • Node or ECMAScript invent a new "weak" or "optional" import attribute (e.g. import fs from "node:fs" with { optional: true }) could be added; if the module fails to resolve, the imports are all undefined. This may require some sort of TC39 specing or proposal.
    • In the meeting this, this seemed palatable, though potentially had a too-long time horizon to be helpful for TS or other projects trying to do the same thing as us.
  • Node can add import.meta.require. This was previously proposed at Pull request opened for import.meta.require on core modules#130, but unfortunately drags CJS into the ESM world (potentially no more than createRequire, I suppose).
    • In the meeting, this seemed "okay" in that it's something Node could offer "now", but less desirable than other options.
  • Node (+ whoever owns the import.meta spec) can add import.meta.importSync or import.now, which is effectively just await import(...) that only works on sync-loadable ESM.
    • In the meeting, this seemed less palatable than other options without a more general use case or more supporting examples.
  • Node can add import.meta.builtins or similar (e.g. on process), which just provide access to Node's builtin modules. Largely, TS only needs fs, path, os, etc, so this would sidestep the "sync import" problem altogether. TS also conditionally imports source-map-support, so that would not work, though only in development. Thankfully, since one could get node:module this way, you can also shim require via createRequire, which is pretty neat.
    • In the meeting, this seemed pretty palatable. There was mention of this being something useful for WinterCG or similar to WASM, but I'm not very qualified to fully understand that one. There was also discussion about whether or not it would be all-getters, since people do want to patch / mock builtins.

What alternatives have you considered?

TS could also use package.json import maps to achieve "conditional" imports of Node-specific code, e.g. have an import like #system which in the Node condition imports from Node, but is shims otherwise. This seems to have a number of downsides in my view, specifically:

  • TS is bundled and our outputs are not associated with our inputs, so actually writing said code may be pretty challenging.
  • There are platforms other than Node that implement enough of the Node builtins to be compatible. Would they set the node condition? Would TS need to explicitly add mappings for each of these? What happens if our code is bundled? These gotchas make me feel like this would be too difficult to deal with.
  • Do import maps work when TS is loaded via a browser? Our intent is that we can go down to having only one copy of our code, but I don't think browsers understand what to do with package.json import maps. In the meeting, @JakobJingleheimer mentioned that one could remap imports like node:fs to shims, even to data:... blobs, but I'm definitely not experienced enough in browser ESM to know how to do that.
@jakebailey jakebailey added the feature request Issues that request new features to be added to Node.js. label Apr 19, 2024
@nicolo-ribaudo
Copy link
Contributor

Given that it doesn't depend on the context, import.meta.builtins could also be process.builtins.

@guybedford
Copy link
Contributor

Thanks for the summary here, in terms of timelines I just want to summarize my own sense of things:

  • import.now - there is committee discussion and interest here, so it isn't unviable, but there's a lot of implementation edges, so we're likely looking at at least a year or so before a viable proposal could even reach Stage 2, and another year or two to get to implementation, if it happens at all.
  • with { optional: true } - this would need to be tackled as a web or ecma spec, but could certainly be done in a timeline closer to within 4-8 months.
  • import.meta.getBuiltin - Node.js could implement and ship this any day.

Perhaps in time we do hit all three, or at least two of the above.

@littledan
Copy link

process.builtins SGTM. I don't see the reason to put extra stuff in the import.meta namespace, which is shared across environments and runs the risk of collisions. Weak imports seem like a good medium-term improvement, but let's do those at the TC39 level (since this feature makes sense across environments).

@jakebailey
Copy link
Author

jakebailey commented Apr 19, 2024

Something on process sounds totally fine to me; I also didn't realize when writing the above that you could also do:

const require = process.getBuiltin("node:module").createRequire(import.meta.url)

And be no worse off than before for conditionally loading anything outside of the builtins, which is very handy.

@jakebailey
Copy link
Author

jakebailey commented Apr 19, 2024

I think the only concern I have for a process-based approach is how bundlers won't recognize these as being "imports" per se, so may not be able to shim them. But I suppose that bundlers would need to change no matter what option is implemented, since it's all new besides import.meta.require already being implemented in bun.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 19, 2024

I think whether the access is module-based depends on #52575 (if policy is enabled, access control to builtins is different per module based on the policy. But then if policy is removed altogether...there is no need to worry about it. It's probably another reason to remove policy?)

@joyeecheung

This comment was marked as resolved.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 20, 2024

Actually thinking through the implementation details of how to implement per-module access control for import.meta.builtins I realize that globalThis.process.builtins can still do it too, in the end something can be stored in the script context similar to the host-defined options used to implement import() (or at least that's what V8 plans to do), and in the native layer all the bindings can use this thing to determine resource control scopes for the module that invokes the builtin. That's going to be a lot more robust than doing it in the module loader anyway. So my conclusion is, globalThis.process.builtins won't shut the door for per-module access control, it's still possible to have both.

@RedYetiDev RedYetiDev added the loaders Issues and PRs related to ES module loaders label Apr 20, 2024
@jakebailey
Copy link
Author

I'm working on a proof of concept; my quick hack for now (until I can get all of it as ESM) is to use a --require preloaded script to patch process and a polyfill stuck on top of our bundle.

$ cat patchProcess.cjs 
process.createRequire = require("node:module").createRequire;
$ head ./built/local/tsc.mjs
var require, __filename, __dirname;
if (typeof process !== "undefined") {
  require = process.createRequire(import.meta.url);
  __filename = require("node:url").fileURLToPath(new URL(import.meta.url));
  __dirname = require("node:path").dirname(__filename);
}
$ node --require ./patchProcess.cjs ./built/local/tsc.mjs -p ./src/compiler --diagnostics
Files:              213
Lines:           245632
Identifiers:     412592
Symbols:         256043
Types:           103601
Instantiations:  189818
Memory used:    496843K
I/O read:         0.01s
I/O write:        0.00s
Parse time:       0.90s
Bind time:        0.38s
Check time:       7.15s
Emit time:        0.00s
Total time:       8.44s

Just throwing on createRequire is a good trick for now, given our code will just try and use require globally if it can. Rewriting everything to assume ESM is definitely possible, but will take a little more work than the few minutes I spent on this showing that it's possible to avoid TLA and yet still conditionally do things.

@jakebailey
Copy link
Author

So, that was a lot easier than I expected.

$ cat testRequireESM.cjs                                                                                                                                                                                                                       // Inject this into the process so that TS can synchronously require stuff.
process.createRequire = require("node:module").createRequire;

const ts = require("./built/local/typescript.js");
console.log(ts.version);
$ node --experimental-require-module ./testRequireESM.cjs
5.5.0-dev
(node:33109) ExperimentalWarning: Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

image

The only change to TS I need to make is to change our import extensions and fix up our system implementation to require process.createRequire. Other stuff is broken (in fixable ways), but this seems to show that a TLA-free ESM TypeScript API is possible without breaking downstream users.

Code is at: https://github.com/jakebailey/TypeScript/tree/its-esm

@joyeecheung
Copy link
Member

Opened #52762

@GeoffreyBooth
Copy link
Member

@nodejs/loaders

@luxaritas
Copy link

luxaritas commented May 1, 2024

Want to throw in a couple of non-builtins use cases I've come across, in case it's helpful:

  • In @opentelemetry/resources, there is a conditional import based on process.platform, so that the correct function depending on perform can be exported while being contained in a different module.
  • Something I've done is in a shared eslint config, conditionally importing either a typescript shared config or a base config depending on whether or not I've "enabled" it, allowing the external TS config to be an optional dependency.

@jakebailey
Copy link
Author

In the first one, you are already importing process; is this actually a conditional?

For the latter, you can actually export from ESLint configs a Promise, such that you can conditionally resolve things in an async IIFE, which may also work for those cases.

No matter what, though, you can always achieve loading of external modules even with just getBuiltin just by writing:

const require = process.getBuiltin("module").createRequire(import.meta.url);
// ...

@luxaritas
Copy link

luxaritas commented May 1, 2024

In the first case, I was referring to the require calls (exceptionally poor choice of words, sorry!). In both cases it didnt register that createRequire should be able to solve this issue - though WRT larger standards discussion I imagine it would be nice to have a way to do this in ESM outside of node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders
Projects
Status: Pending Triage
Development

Successfully merging a pull request may close this issue.

8 participants