-
Notifications
You must be signed in to change notification settings - Fork 44
Pull request opened for import.meta.require on core #130
Comments
@guybedford had voiced an objection to the feature in the pull request
My response
Moving conversation here so we can focus on technical implementation in the PR |
+1 to removing transparent interop. IMO, removing it will greatly simplify things. I don't think very many modules will transparently upgrade from CJS to JS modules - there's almost certainly going to be breaking API changes around exports. Regarding @guybedford's later comment:
Modules targeting browsers should not ever use |
@justinfagnani that's off topic for this issue; let's keep this thread strictly to |
@ljharb this thread does directly relate to transparent introp as the alternative mechanism for importing CommonJS, to restrict those arguments is to ignore key points. |
@guybedford i agree, but discussing removal of transparent interop is a highly contentious question that will be unlikely to ever attain consensus, and conducting that here will derail the central question of this issue. |
Are you likely to end up in this situation. If React provides an ESM build, why not use the ESM build in Node as well? If a package provides both ESM & CJS, and you're using ESM in your app, would you need to point at the CJS version at all? In this formulation, |
I think the ability to import 'cjs';
import 'esm'; vs import.meta.require('cjs');
import 'esm'; This means that if you have a dependency on a module that is CJS being evaluated in a specific order in your module graph (generally before some dependency in order to configure a singleton / configuration / polyfill / etc.), that This all means, if we can iron out that even with this PR that import cjs from './cjs.js';
const {require, __filename, __dirname} = cjs.require; module.exports = {require, __filename, __dirname}; This PR reduces the number of modules in the graph and reduces the boilerplate above. It also encourages people to use CJS by intentionally exposing the On the note of @mAAdhaTTah . I don't fully understand the comment. Given the ability to On the note of @guybedford , I agree that this PR would be problematic if we were to remove the ability to Finally, regarding minimalism; I think we should be very concise in what we want to ship as a minimal implementation. I think that this PR has merits, but given that I see it as having fewer uses vs the ability to |
@bmeck My apologies for the confusion; the excerpt I quoted could have used more context. I was specifically addressing the browser interop concern raised by @guybedford. I'm suggesting that |
If the goal of So it's either a bad polyfill because it's missing elements of the polyfill or a redundant entrypoint as |
Transparent interop (i.e. the ability to This is true, except for one case—when the require is inside a function and not a "top level" require. In this case, changing the synchronous
So, if it's not already obvious, I'm a big +1 on this. |
@giltayar this can be alleviated as I describe above by importing a function foo(cb) {
require('bar')(cb);
} function foo(cb) {
import('bar').then((ns) => ns.default(cb));
} In particular I want to find how much of the usage is lazy loading regardless of timing, versus lazy loading that must be done synchronously. Even still, sometimes lazy loading is done due to writing things inline, not a requirement on the lazy loading itself. In case it is not clear, I am -1 since this use case can be fulfilled with the ability to [Addendum:] I would like to note that |
My understanding is that this PR allows you to support the following use cases to migrate to ESM more easily:
I tend to avoid these patterns, but they are used because I personally consider that the benefits of ESM modules (static-first nature, async dynamic imports) are very important and want my dependencies to move to pure ESM as soon as possible, even if it requires breaking their API. But it's not realistic, converting larger code bases to ESM and breaking their API in a single step may be impossible for some libraries, and even if I like ESM I'd prefer to avoid a Python-like split where transition issues cause a split for many years. That's why I think that you should be able to cover the use cases above in ESM; but I hope that relying on |
The advantage of (*) It shows up unless somebody goes out of their way to hide it. |
@jkrems you can check if the import in my workaround is a require function statically, I'm not sure how it is different in that sense? The lint/analysis requires it to be done cross file, but the same is true for tree shaking analysis so that kind of thing already seems to be possible. |
@bmeck It's possible but at least afaik implementing an ESLint rule for example would be a lot harder because those tools assume local analysis. |
@jkrems certainly harder, but the use case of static analysis remains. I'm not saying that there isn't value in this PR, just that it is removing boilerplate/difficulty not adding features that I can tell if we have the ability to |
I think there are certain patterns that are being called out and aren't covered by |
@jkrems I'd disagree since you don't have to, just import a |
I don't think "import a require" is a feasible alternative. My thoughts:
|
@ljharb can you expand on what is not feasible about importing a In particular, with Encouraging solutions that work for ESM and encouraging people to move to ESM based |
@bmeck it's wildly unergonomic to get a require function in multiple modules that operates with the same base directory. In other words, if every time I want require in ESM, i have to make a sibling CJS file that does |
I do think it is an interesting possibility of allowing people to create |
Not allowing import declarations to load cjs seems like a very bad idea to me for many reasons that have been brought up already (especially by @bmeck). |
They’re not necessarily exclusive. If you look at the |
It's still worth noting tho that the expectation you can do both only arises from either babel's interop, or from an incorrect belief that named imports is in any way similar to object destructuring. |
Perhaps that’s a bad example. Here’s a better one: if (something)
require('huge-commonjs-package');
|
@GeoffreyBooth iirc there is a proposal somewhere for static import statements in not-top-level places. (it might have also just been some brainstorming on irc, i can't remember) the larger point i want to make, however, is that esm itself can fit into a lot of these issues, without us needing to worry about it. (also https://github.com/guybedford/proposal-dynamic-modules) |
@devsnek I think you may be talking about https://github.com/tc39/proposal-dynamic-import? |
@mkay581 no, I'm meant static import. |
Oh I'd be curious to know how that would be implemented. Do you have the link to the official discussion/proposal? |
Removing from agenda for now, can be re added later |
Closing. PR has been moved to the fork |
Hey all,
I've opened a PR to introduce import.meta.require to core
nodejs/node#21317
I think this feature is needed no matter which overall implementation / feature set we move forward with. I'm adding this to the modules agenda and would like to see us reach consesnsus on landing this if possible.
If individuals have objections or concerns please let me know, either in this thread, the PR, or privately.
The text was updated successfully, but these errors were encountered: