modules icon indicating copy to clipboard operation
modules copied to clipboard

Utility method for exposing Node’s type resolution

Open GeoffreyBooth opened this issue 4 years ago • 24 comments

Split off from https://github.com/nodejs/modules/issues/389#issuecomment-537228049. I think it would be useful to have a function that returns how Node would try to interpret a file, in particular whether Node would try to parse a path to a .js file as CommonJS or as ESM. This would spare tools from needing to reimplement Node’s “find the nearest parent package.json and see if it has a "type" field” algorithm.

Whether the file would be interpreted successfully as that type is irrelevant; it could be a zero-byte file for all this API cares. One use case for this is for tools loading .js config files to know whether to try to load the files via require or import().

GeoffreyBooth avatar Oct 01 '19 22:10 GeoffreyBooth

i guess this is really more like a canRequire api right? since import() can handle everything.

devsnek avatar Oct 02 '19 02:10 devsnek

i guess this is really more like a canRequire api right? since import() can handle everything.

Well first, can import() expressions handle everything that import statements can? I assume yes? But we still need require for .json and .node.

For this particular use case, sure, knowing whether or not require can be used solves the need: if canRequire, then require the file, else use fs.readFileSync to get it as a string and then esm to transpile it and then Module._compile to load it (like with https://github.com/floatdrop/require-from-string). This is for tools that can’t or won’t refactor to use async functions and therefore use import().

But that’s not as useful as just knowing whether Node would treat a particular .js file/path as CommonJS or as ESM. Knowing that would also solve this use case, and open up other possibilities like knowing how to parse the file if a tool wanted to do anything with it (like transpile it etc.). Also it would be more future-proof, for example if require of ESM ever happens—since if it does, would require still be synchronous or would it return a promise like import()?

GeoffreyBooth avatar Oct 02 '19 05:10 GeoffreyBooth

Also @devsnek please see https://github.com/eslint/eslint/pull/12333#issuecomment-536850920 for a pretty extensive list of gotchas that one user ran into when trying to replace require with import(). Unless there are workarounds or answers for all of those cases, that user is going to need to know when to use require instead of import(). (And if you don’t mind commenting on that thread and answering those questions, I’d appreciate it.)

GeoffreyBooth avatar Oct 02 '19 05:10 GeoffreyBooth

after all is said and done we should be able to load json and native add-ons with import too. the process of migration is always going to be tough.

devsnek avatar Oct 02 '19 14:10 devsnek

I’m confused... import() can load a CommonJS file? Maybe I’m misreading the docs.

I need to load a .js file, from a CommonJS file, and I don’t know what it is, so I don’t know whether to use import() or require(). Ideally, I could give a function a filepath and it could tell me the type of that file. Or, even better, it could just do the right thing (and return a Promise, I suppose)

boneskull avatar Nov 25 '19 20:11 boneskull

import() can load a CommonJS file

Yes, import() and import can each load both CommonJS and ES module files.

GeoffreyBooth avatar Nov 25 '19 21:11 GeoffreyBooth

Duplicate of https://github.com/nodejs/node/issues/30514

GeoffreyBooth avatar Nov 25 '19 21:11 GeoffreyBooth

@GeoffreyBooth, I presume needs are met for this issue as well. Should it be closed?

DerekNonGeneric avatar Jan 07 '20 00:01 DerekNonGeneric

@GeoffreyBooth, I presume needs are met for this issue as well. Should it be closed?

I don't think so. The other issue was specific to the context of loaders, where the needs are met; but the issue here is more general, and covers user application code (like a build tool needing to know how to load a file). The defaultGetFormat function mentioned in the other issue is only available to loaders, not application or library code.

GeoffreyBooth avatar Jan 07 '20 05:01 GeoffreyBooth

Gotcha, thanks.

DerekNonGeneric avatar Jan 07 '20 05:01 DerekNonGeneric

Jest would very much use such a function, and I'd love to not have to implement the logic manually. We implement our own require (for mocking and dependency tracking, and to run/evaluate them in different vm.Contexts) and will use custom linking etc for ESM support when we get that far (see https://github.com/facebook/jest/issues/9430). We'd need to know to use vm.Script or vm.Module when loading a file - both from user space and from node_modules. A utility function from node that can figure that out for us would simplify things for us.

SimenB avatar Jan 20 '20 13:01 SimenB

@SimenB I believe what you need is not to figure out whether it's esm or cjs, but rather what does node's module resolution think the file is, according to its module resolution algorithm.

For example, let's say you have a test.js file that has no imports and is in a folder with a package.json with type:module. What node would do is treat it as esm. Even if it has a "require", node would treat it as esm. And what we would expect from jest is to treat it as esm and "import" it and not "require" (or the equivalent you will use with "vm")

I added esm support for Mocha, and the trick I used to figure that out was to require the module, and if I got an error from node that says that it's an esm, then I import it. A hack, but given that there is currently no way to figure it out (besides duplicating nodes algorithm), then that is the only solution I could find.

giltayar avatar Jan 22 '20 17:01 giltayar

I added esm support for Mocha, and the trick I used to figure that out was to require the module, and if I got an error from node that says that it's an esm, then I import it.

Why not do it the other way around? import() can accept either ESM or CommonJS, so it should just work on the first try without you needing to catch an error and fall back to an alternative.

GeoffreyBooth avatar Jan 22 '20 17:01 GeoffreyBooth

Why not do it the other way around?

Not who you asked but I assume because it would break synchronous consumers that may not support async test suite setup. Starting with require means the change in behavior only affects people who actively use modules.

P.S.: But I agree that in the general case starting with ESM will be safer, especially in a future where loaders may make it impossible ("harder than reasonably implementable") for CJS to fail when ESM is required.

jkrems avatar Jan 22 '20 17:01 jkrems

Yeah, I'm hoping the function discussed in the OP will allow us to not have to implement guessing or fallbacks. The use case of loading config presented in the OP is not my main use case, but rather custom implementation of require and import. We can of course look at file extensions and use https://github.com/sindresorhus/read-pkg-up and inspect the type field ourselves, but since node already implements this logic it'd be wonderful to not have to roll our own 🙂

SimenB avatar Jan 22 '20 19:01 SimenB

@GeoffreyBooth - @jkrems should have been correct, but since we have two paths currently in Mocha—an async one that supports both esm and cjs, and a sync one that supports only cjs and is there for api backward compatibility, then that wasn't a problem for us.

No, the real reason is that I was chicken. 😀Mocha is used by millions of tests around the world, and just thinking that suddenly they're all going to pass through the newly deployed import made me, frankly, a bit worried. We will probably switch to using import, as you said we should, but we really want the ESM mechanism to get some real time in production!

giltayar avatar Jan 23 '20 08:01 giltayar

I added esm support for Mocha, and the trick I used to figure that out was to require the module, and if I got an error from node that says that it's an esm, then I import it.

Why not do it the other way around? import() can accept either ESM or CommonJS, so it should just work on the first try without you needing to catch an error and fall back to an alternative.

The ExperimentalWarning: The ESM module loader is experimental. is a problem here. Loading a CJS with import() still produces the warning, I assume mocha (and any other tool) does not want to cause this warning unnecessarily.

coreyfarrell avatar Jan 23 '20 08:01 coreyfarrell

@GeoffreyBooth look into my esm-loader implamentation to load ESM from string https://github.com/direktspeed/esm-loader

frank-dspeed avatar Jan 23 '20 11:01 frank-dspeed

@GeoffreyBooth and by the way to get the needed informations you can use the new added esm loader hooks they deliver all the information like module resolve algos, https://nodejs.org/api/esm.html#esm_experimental_loaders

there are some new added hooks

frank-dspeed avatar Jan 23 '20 11:01 frank-dspeed

@coreyfarrell - this was true up to Node 13.3. From Node 13.4 the warning was removed. This I verified experimentally...

giltayar avatar Jan 23 '20 14:01 giltayar

@giltayar the warning was not produced for dynamic import in 13.3.0 but this was fixed in 13.4.0 by nodejs/node#30720. If you are able to use import() in 13.4.0+ with no experimental warning I'd be interested in seeing how.

coreyfarrell avatar Jan 23 '20 15:01 coreyfarrell

@coreyfarrell I may have confused two warnings. I wasn't talking about the warning about ESM being experimental, but rather of this warning:

(node:3452) Warning: require() of ES modules is not supported.
require() of C:\test.js from C:\...\npm\node_modules\mocha\lib\esm-utils.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename test.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from C:\package.json.

This warning is very problematic for Mocha, and happily was was removed in Node.js 13.4.

giltayar avatar Jan 29 '20 12:01 giltayar

I've just published get-package-type v0.1.0 for this purpose. I need this functionality for nyc to support loading nyc.config.js where type: module applies.

This package was implemented from scratch since the node.js implementation is in C++ so I was unable to trace out exactly what that does. I've opened a couple issues seeking feedback to things that I think need to be resolved before I can bump to v1.0.0. I'd appreciate any comments on those issues. I've also opened cfware/get-package-type#6 for any general comments/discussion as I don't want to take over this thread.

coreyfarrell avatar May 19 '20 10:05 coreyfarrell

This issue would have to deal w/ CLI arguments potentially from https://github.com/nodejs/node/issues/37848

bmeck avatar Mar 23 '21 13:03 bmeck