Closed MylesBorins closed 6 years ago
@guybedford had voiced an objection to the feature in the pull request
I'm -1 on this for two reasons:
- The way that a module is imported has to change depending on the module format. This puts cognitive overhead on the user above just "import a module".
- Code analysis, transpilers, and bundlers will need to adapt to this. For example Rollup can do module analysis due to the bindings nature of ES modules. If import.meta.require is required to be supported in Rollup (which would be the case for CommonJS modules being imported this way in the Node ecosystem), then this could interfere with the current treeshaking benefits Rollup provides.
My response
@guybedford to your two points
The way that a module is imported has to change depending on the module format. This puts cognitive overhead on the user above just "import a module".
There will be a follow up PR to this in which I plan to propose removing transparent interoperability. It seems to me like this specific point only becomes an issue if we remove that capability. In the mean time, with the current implementation, individuals are free to statically import a cjs module or dynamically import it. If people are planning to dynamically bring in CJS it does seems odd to require them to wrap it in a process via dynamic import.
When I propose removing transparent interoperability I plan to implement a way to trap errors related to importing common.js and give extremely clear errors about how to use import.meta.require. As it stands right now people already need to know the "type" of the module as CJS cannot do named exports. I personally ran into this exact problem trying to
import
our test common module. The current behavior is not very clear and one has to know not only the type of module, but the specific edge case to our implementation as well as the fact that there is a difference between named exports and destructuring. I do not consider that simpler.Code analysis, transpilers, and bundlers will need to adapt to this. For example Rollup can do module analysis due to the bindings nature of ES modules. If import.meta.require is required to be supported in Rollup (which would be the case for CommonJS modules being imported this way in the Node ecosystem), then this could interfere with the current treeshaking benefits Rollup provides.
While I will agree that this is a potential edge case I am not convinced that this specific case significantly breaks expectations. People are already using both
import
andrequire
liberally in their code being transpiled and bundled by tools like rollup and webpack. These tools already need to recognize therequire
keyword, and it would seem that identifyingimport.meta.require
would be fairly similar. I'd like to here from @Rich-Harris and @thelarkinn about their thoughts. I for one think that being explicit in these cases, and removing the ambiguity around import (if we eventually remove transparent interop) would eventually simpify tree shaking algorithms. I am very open to being mistaken on this.
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:
Another important point here is that here is no way for browsers to provide import.meta.require. One could imagine import react From 'react' being pointed to a CDN source on the web, and from CommonJS in Node, but such a mapping system isn't possible with import.meta.require - the code would not work in the browser without a build step that involves altering the code of the module.
With transparent interop on the other hand, something as simple as a package name map could provide this support.
Modules targeting browsers should not ever use import.meta.require
as there's no way for browsers to implement require()
's synchronous semantics anyway. It would only ever be a feature of bundlers. Package name maps would only let code resolve a name to a URL, not fetch an asset and it's transitive dependencies synchronously.
@justinfagnani that's off topic for this issue; let's keep this thread strictly to import.meta.require
.
@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.
One could imagine import react From 'react' being pointed to a CDN source on the web, and from CommonJS in Node ... With transparent interop on the other hand, something as simple as a package name map could provide this support.
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, import.meta.require
is useful primarily for importing packages that only have a CJS build, which is going to require a build step anyway.
I think the ability to import
CJS has a very important note here. import.meta.require
does not satisfy all the use cases of being able to import
CJS. I don't think we should associate this PR as a means to remove import
ing CJS. In particular the ability to import
CJS allows for side effect / singleton dependencies to interleave with ESM dependencies. e.g.
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 import.meta.require
cannot fulfill that use case. There are a variety of workarounds if you start using import()
and await
in conjunction with import.meta.require
to allow such evaluation ordering, but that seems a problematic integration/migration story.
This all means, if we can iron out that even with this PR that import
ing CJS is needed to fulfill various use cases, we should discuss the value that this PR brings. In particular, with the ability to import
CJS, you can obtain a require()
function by importing it from CJS:
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 require
function. We should focus on the reduction in boilerplate that this provides, and the intention of keeping require
as a vital part of ESM modules in Node.
On the note of @mAAdhaTTah . I don't fully understand the comment. Given the ability to import
CJS why would you not want to import
React if it had both ESM and CJS? Why would you not want to import
a library if it was only CJS? If path searching is preserved for example, you can even have a migration path that does not require that people select which format the library is in. import 'react';
is able to refer to both ESM and CJS. If the path searching algorithm selects ESM prior to CJS / JSON / etc. you would still get the ESM build.
On the note of @guybedford , I agree that this PR would be problematic if we were to remove the ability to import
CJS and that import.meta.require
is harder to analyze than import
. Given my statements above, I do not think we can remove the ability to import
CJS with this PR as it does not fulfill all the uses of being able to import
CJS. I do think we could make it have fewer bailout conditions if we make import.meta.require
non-configurable, but agree that it will still be very hard to analyze.
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 import
CJS and mostly appearing to be a boilerplate removal I would prefer it be discussed as a follow on to any initial implementation rather part of the initial implementation.
@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 import.meta.require
doesn't impact browser interop because regardless of whether you import
or import.meta.require
a CJS module, a build step will be required to make it work in the browser. You're not going to run into a situation where you'd map the browser import statement to an ESM module and the Node import statement to a CJS one. If you have an ESM module, you'd just use it both.
If the goal of import.meta.require
is to provide a way to bail on using esm semantics and fallback to commonjs behaviors (so you can do less work to port older code), removing all the members on require
seems like a mistake - people depend on require
's ancillary functions; that's why they're there (and I'm going to reiterate this over and over again, exposing things to esm is not a "good time" to drop older APIs - either its for compatibility or its not). If import.meta.require
isn't actually require
, then it's not nearly as useful for porting code - it's a 3rd kind of code-running construct, separate from require
in cjs
or import
s in esm
. If all it can do is run cjs
code, its only use is as a way to... run cjs
- meaning if implemented without its members, the only purpose is to synchronously run cjs
or attempt to subsume import
's job of running cjs
.
So it's either a bad polyfill because it's missing elements of the polyfill or a redundant entrypoint as import
s can do the same thing. So at least one of those should be true for it to have value; either import
shouldn't work on cjs
(đź‘Ž ) , or it should have all the members a cjs
consumer would expect require
to have to it presents the same API it used to (despite being at a different identifier in esm
for unfathomable reasons).
Transparent interop (i.e. the ability to import
a CJS module) enables a developer to move all their requires
to import
-s easily (and one can argue whether this is a good or a bad thing).
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 require
to an async await import
(or the equivalent promise code) will turn that function from a synchronous function to an async one, which may mean a significant refactoring of the code base, and which will impede the migration of that codebase to ESM.
import.meta.require
will help this migration by enabling the migration process to procede in an incremental fashion and not require major refactorings in the initial—and mostly technical—phase, of turning all require
-s into import
-s.
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 require()
function from a CJS file. I agree that there is merit to this, but I think we can leave this out of the initial implementation and see how common it is for people to use the functionality of require()
vs import
. Even if the require()
is in a nested function
scope, it may be fine to use import
:
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 import
CJS still. The boilerplate is heavier to import
a require()
function, but the ability to lazy load is still there through import()
and the main question is if people need the lazy loading to be synchronous in a large enough usage.
[Addendum:] I would like to note that require()
would require all lazy loaded dependencies be CJS in order to load synchronously.
My understanding is that this PR allows you to support the following use cases to migrate to ESM more easily:
// A common pattern before promises became part of the language was
// to let users provide their own Promise impl or fall back to a default lib if
// none was provided.
// It's currently possible to require the default library synchronously
// (for example in a constructor)
class Db {
constructor(options) {
if (options.promiseLib !== undefined) {
this.promiseLib = options.promiseLib;
} else {
this.promiseLib = require("bluebird");
}
}
// Other examples include loading a different module based on an env variable
function getSchema(name) {
return require(`./${name}-schema`);
}
const userSchema = getSchema("user"); // require("./user-schema");
const itemSchema = getSchema("item"); // require("./item-schema");
// For example a lib has an optional dependency: trying to require it may fail so it
// wants to catch the error and use a fallback
let superCrypto;
try {
superCrypto = require("super-crypto-native");
} catch (err) {
superCrypto = require("super-crypto-js");
}
let mimeDb = null;
function getMediaType(extension) {
if (mimeDb === null) {
mimeDb = require("mime-db");
}
return mimeDb.getByExtension(extension);
}
I tend to avoid these patterns, but they are used because require
currently allows them. I don't think that you can port these modules to ESM using only spec-compliant import
and import()
.
Switching to import()
would require all these use-cases to be become asynchronous. Due to the contagious nature of async function, it will certainly require a breaking change for library API. For some cases it may even require larger changes: a constructor must be synchronous so you cannot just fix my first example with an await import();
.
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 require
will only be used for the transition period and not become a widespread pattern.
I was originally in favor of this PR, but following @bmeck's comment I am no longer sure if it's really needed if you can statically import CJS from ESM. His solution where you get require
into ESM from CJS lets you quickly solve the examples I provided above, but it also looks clearly like a workaround that calls to be refactored as soon as possible. So you can support synchronous require without promoting it as a first-class feature. (Another solution would be to plan to deprecate import.meta.require
a few years after the transition is complete, but once it's there you won't be able to get it out; or remove interop from ESM to CJS so the workaround is no longer possible).
The advantage of import.meta.require
(as opposed to an ad-hoc implementation of a workaround) is that import.meta.require
shows up(*) in static analysis. As in: you can easily lint for it and you can easily tell that a certain file was included in a way that won't necessarily work in other ESM environments. Which I think is valuable in itself.
() It shows up unless somebody goes out of their way to hide it. `getProp(import.meta, 'require')(/ ... */)` is possible but I would argue unlikely.
@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 import
CJS.
I think there are certain patterns that are being called out and aren't covered by import
of CJS, at least if our goal is to minimize the need to fall back on CJS. See "Synchronous optional dependency imports / failable imports" for example. Falling back if an (optional) native dependency isn't available isn't covered by import
of CJS. Moving that logic to a CJS file doesn't count imo.
@jkrems I'd disagree since you don't have to, just import a require
if you want. The introduction of import.meta.require
ensures the survival of require
as part of our ESM implementation and encourages its use vs alternatives. I do think ways to fallback are being investigated that do not involve require
such as Layered APIs. We should work to fix the use cases using ESM rather than telling people to use CJS I think. The topic of how to solve this using ESM is not clear, so using CJS temporarily should be a good approach. I just don't see import.meta.require
as temporary nor as a way to encourage people to use ESM, quite the opposite in fact.
I don't think "import a require" is a feasible alternative. My thoughts:
import 'cjs'
covers all the static top-level use cases of require
import.meta.require
is still useful even with import 'cjs'
import.meta.require
and a static require
are 100% identically statically analyzeable, and there is no excuse but "we haven't built it yet" for any ESM-based treeshaking or analysis that can not also identically apply to static-CJS-based treeshaking or analysisimport.meta.require
per above, but NOT in favor of it if it's going to be used as ammunition in any arguments against import 'cjs'
.@ljharb can you expand on what is not feasible about importing a require()
function? I'm in agreement that having access to require()
does have value, but am trying to keep our initial implementation minimal, and trying to ensure that we don't introduce a permanent API for a temporary problem.
In particular, with require()
either from import {require} from 'cjs'
or import.meta.require
, all of the cases described above about synchronous loading, are only towards dependencies that can be loaded synchronously (module graphs that are only CJS). As ESM becomes more widespread that possibility will be reduced as loading ESM exported values cannot be done synchronously. Therefore, tailoring to the experience of synchronous loading should heavily be considered as a temporary situation as the future will decrease the viability of relying on those situations.
Encouraging solutions that work for ESM and encouraging people to move to ESM based import()
is not universal, but inevitable as people have noted about how the async nature of things is viral. We should aim to assist people during the transition period until workflows emerge and refactoring can occur to solve use cases using ESM. I am not convinced we should tailor our experience to a transition phase in a way that gives us permanent features of CJS inside of ESM.
@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 module.exports = require
, that would be a massive burden. (If I can import it in a standard way, then that's fine, but there's no current facility to provide source-code-contextual information via an import, except via import.meta
)
I do think it is an interesting possibility of allowing people to create require()
functions themselves. I've seen people have general interest in creating require()
functions, and think exposing makeRequireFunction
or similar would serve not just the ability to get a hold of require()
in a more ergonomic way, but also add possibilities for people wishing to run require()
with a different directory from the resolution point. I'd be +1 on putting something like that on the 'module'
module.
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).
Not allowing import declarations to load cjs
They’re not necessarily exclusive. If you look at the --experimental-modules
implementation, import
statements of CommonJS files can’t do everything that traditional CommonJS require
statements can do; you can do const { shuffle } = require('underscore')
, for example (where underscore
is CommonJS) while you can’t do import { shuffle } from 'underscore'
. There are reasons we might want to allow both.
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');
import
statements may only appear at the top level.
@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)
iirc there is a proposal somewhere for static import statements in not-top-level places.
@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
https://github.com/nodejs/node/pull/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.