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

Utility method for exposing Node’s type resolution #49446

Closed
GeoffreyBooth opened this issue Oct 1, 2019 · 27 comments
Closed

Utility method for exposing Node’s type resolution #49446

GeoffreyBooth opened this issue Oct 1, 2019 · 27 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. stale

Comments

@GeoffreyBooth
Copy link
Member

Split off from nodejs/modules#389 (comment). 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().

@devsnek
Copy link
Member

devsnek commented Oct 2, 2019

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

@GeoffreyBooth
Copy link
Member Author

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
Copy link
Member Author

Also @devsnek please see eslint/eslint#12333 (comment) 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.)

@devsnek
Copy link
Member

devsnek commented Oct 2, 2019

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.

@boneskull
Copy link
Contributor

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)

@GeoffreyBooth
Copy link
Member Author

import() can load a CommonJS file

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

@GeoffreyBooth
Copy link
Member Author

Duplicate of #30514

@GeoffreyBooth GeoffreyBooth marked this as a duplicate of #30514 Nov 25, 2019
@GeoffreyBooth
Copy link
Member Author

@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.

@SimenB
Copy link
Member

SimenB commented Jan 20, 2020

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 jestjs/jest#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.

@giltayar
Copy link
Contributor

@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.

@GeoffreyBooth
Copy link
Member Author

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.

@jkrems
Copy link
Contributor

jkrems commented Jan 22, 2020

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.

@SimenB
Copy link
Member

SimenB commented Jan 22, 2020

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 🙂

@giltayar
Copy link
Contributor

@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!

@coreyfarrell
Copy link
Member

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.

@frank-dspeed
Copy link
Contributor

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

@frank-dspeed
Copy link
Contributor

frank-dspeed commented Jan 23, 2020

@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

@giltayar
Copy link
Contributor

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

@coreyfarrell
Copy link
Member

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

@giltayar
Copy link
Contributor

@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.

@coreyfarrell
Copy link
Member

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.

@bmeck
Copy link
Member

bmeck commented Mar 23, 2021

This issue would have to deal w/ CLI arguments potentially from #37848

@GeoffreyBooth GeoffreyBooth added the feature request Issues that request new features to be added to Node.js. label Sep 1, 2023
@GeoffreyBooth GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label Sep 1, 2023
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Sep 1, 2023
@GeoffreyBooth GeoffreyBooth transferred this issue from nodejs/modules Sep 1, 2023
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 29, 2024
@SimenB
Copy link
Member

SimenB commented Feb 29, 2024

I still definitely want this, but the implementation baked into jest-resolve seems to be correct (I haven't gotten any bug reports on it, at least).

@github-actions github-actions bot removed the stale label Mar 1, 2024
@frank-dspeed
Copy link
Contributor

frank-dspeed commented Mar 4, 2024

@SimenB @GeoffreyBooth as the issue got closed: see: eslint/eslint#12333 (comment)

but without a result i want to supply some impotent internal insights.

  • The ESM System has a internal module cache that is not exposed by design. It is Garbage Collected fully system internal managed.
  • A ESModule gets loaded only once and then stays in memory thats why the tests did not re-run to re run them you need to invalidate the cache this is done via url patterns as in the browser import('./my.js?cache=xxxx') this is a example using a server query string but you can also use simple url hashes import('./my.js#1') import('./my.js#2') import(`./my.js#${new Date().toISOString()}`)
  • The only way to clear the v8 Module cache is to throw away the whole isolate
  • for frontend module loaders where i want explicit garbage collection control i need to create a whole new context that i can throw away and connect the scripts via postMessage or other internal messaging.

High Level Picture

The Browser Platform Manages the life cycle of the isolates that loaded the modules eg: workers lifecycle design which inherent
schedules regular termination of the worker when not needed. Or Webpages that are not in focus for a longer time. As the internal code is not aware of the outer Embedder the design choice was to not expose the cache.

Saw the resolution part to late like the experimental flag for auto module types

parsing the file for import as static syntax and export as static syntax is the only way to say if something is a module or not

// is a module if it contains import or export keyword not followed by ( as next char to it no matter how much spaces or new lines are inbetween 

https://nodejs.org/dist/latest-v21.x/docs/api/cli.html#--experimental-detect-module

the --experimental-detect-module simple uses the same that typescript uses in classic resolve so both systems would have same behavior.

Copy link
Contributor

github-actions bot commented Sep 1, 2024

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
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. feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests

9 participants