Import.meta.require #21317

MylesBorins · 2018-06-13T18:03:23Z

Hey All,

This builds on top of @targos's earlier implementation of import.meta.require. It has been refactored to work with our current layout for the module system. I've also added some basic tests, let me know if you think we need something more robust.

@nodejs/modules I am not planning to land this without consensus from the group, but I think that this is a necessary base feature no matter what implementation we move forward with. I'll open a corresponding issue to discuss.

nodejs-github-bot · 2018-06-13T18:03:25Z

@MylesBorins build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/55/pipeline

MylesBorins · 2018-06-13T18:03:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15440/

MylesBorins · 2018-06-13T18:04:31Z

Marking as semver-minor for now, but we might want to remove the label due to modules still being experimental.

ljharb · 2018-06-13T18:08:52Z

As long as import.meta.require uses the same algorithm that CJS require does - whether that includes transparent ESM interop or not - then I agree that this is critical to have available in ESM.

(There's plenty of use cases for conditional synchronous/immediate/same-tick imports that the JS language doesn't provide for, and that includes being able to conditionally bring in ESM)

MylesBorins · 2018-06-13T18:09:48Z

@ljharb this implementation uses the exact same mechanism to create the module / require object as we use in the CJS IIFE.

TLDR, this require is identical from the require in CJS and will inherit any changes without us having to make changes in this implementation... it is an instantiation and assignment.

guybedford · 2018-06-13T18:14:21Z

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". Also upgrading a module from CJS to ESM requires changing the way it is imported.
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.

Edit: 3. Universal workflows - #21317 (comment)).

ljharb · 2018-06-13T18:28:25Z

@guybedford ftr, it wouldn't interfere with those benefits if rollup took the time to implement them for static CJS, which is identically as tree-shakeable as ESM - iow, the only "interference" would be because rollup only has partial support for treeshaking modules - where "modules" includes both ESM and CJS.

devsnek · 2018-06-13T18:46:43Z

lib/internal/process/esm_loader.js

+        if (req !== undefined)
+          return req;
+        const url = new URL(meta.url);
+        const path = url.pathname;


use getFilePathFromURL from internal/url here

devsnek · 2018-06-13T18:48:46Z

test/es-module/test-esm-import-meta.mjs

@@ -3,20 +3,38 @@
 import '../common';
 import assert from 'assert';

+const fixtures = import.meta.require('../common/fixtures');


this should be exposed by common.mjs

I was thinking that using it this way offered an extra smoke test for import.meta.require.

more than happy to change it though

at best we can take it as an excuse to build up common.mjs... i don't like the idea of "smoke tests" but that's just an opinion.

mcollina

LGTM

mcollina · 2018-06-13T18:51:46Z

I think this PR should include the documentation about this feature.

bmeck · 2018-06-13T19:00:39Z

If we do go through with this, I'd prefer we remove some things that might get confusing like .cache, .extensions, .main that can cause some confusion to myself at least since ESM doesn't use those. require.resolve seems ok to me, but the reflection into CJS people might expect to work on the current module which it won't in the same way as CJS modules.

ljharb · 2018-06-13T19:03:57Z

require.resolve uses require.extensions, and it will break a bunch of use cases if import.meta.require doesn’t respect them.

jkrems · 2018-06-13T19:08:47Z

👍 for removing the properties if they exist (e.g. import.meta.require.extensions) even though I agree with @ljharb that import.meta.require should work like the "normal" require and honor the extensions mechanism. But I don't think we should allow setting up .extensions properties from ES6 modules.

bmeck · 2018-06-13T19:09:28Z

@ljharb it would continue to function off the Module._extension like other instances of require : https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L120 , I'm just saying we should not expose the properties for inspection/mutation.

MylesBorins · 2018-06-13T19:14:55Z

@bmeck would you like to push a fixup commit to remove the properties you think shouldn't be exposed?

MylesBorins · 2018-06-13T19:24:22Z

@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 and require liberally in their code being transpiled and bundled by tools like rollup and webpack. These tools already need to recognize the require keyword, and it would seem that identifying import.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.

edit: @guybedford I've moved the copy from our conversation over to the modules issue as I think that might be a better place to have the conversation... leaving this issue to focus on technical merits of the implementation.

Feel free to respond in either thread

MylesBorins · 2018-06-13T19:35:27Z

One more CI, should hopefully fix windows error

https://ci.nodejs.org/job/node-test-pull-request/15443/

@mcollina I've made some basic docs changes. LMK what you think. Will expand as implementation is clearer

jkrems

🎉

guybedford · 2018-06-13T19:48:23Z

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.

MylesBorins · 2018-06-13T19:49:50Z

Ok, one more CI now that the properties have been removed as requested above. I've also documented these. If anyone has ideas on how to improve docs please feel free to push a fixup commit

CI: https://ci.nodejs.org/job/node-test-pull-request/15444/

guybedford

I'm against this so long as we can support the same use cases through transparent interop, for reasons as described in #21317 (comment).

jdalton · 2018-06-13T20:16:24Z

lib/internal/process/esm_loader.js

+        delete req.cache;
+        delete req.extensions;
+        delete req.main;
+        delete req.paths;


req.paths do you mean req.resolve.paths?

Given this confusion - would it be better to keep this to a minimal "only expose the pure function without any additional properties"? E.g. import.meta.require = actualRequire.bind(null) to get rid of all potential properties..?

EDIT: This was meant as a "for the initial implementation".

MylesBorins · 2018-06-13T20:40:57Z

One of @jkrems questions got lost in a refactoring

Given this confusion - would it be better to keep this to a minimal "only expose the pure function without any additional properties"? E.g. import.meta.require = actualRequire.bind(null) to get rid of all potential properties..?

EDIT: This was meant as a "for the initial implementation".

bmeck · 2018-06-13T20:44:28Z

People use require.resolve and I'm not sure what all this PR is intending to provide. I just had things I wanted to remove as they seemed problematic if you expect them to affect the current module / ESM.

jdalton · 2018-06-13T20:46:59Z

Keeping things simple, e.g. actualRequire.bind(null), seems like a nice way to start out. It also avoids accidental additions if things are added later to require in general.

bmeck

I'm going to cross ref a recurring comment on PRs that are doable in userland JS that occured in the createRequireFunction PR and affects this one as well : #19360 (comment)

A number of members of core have concerns about adding APIs that can be recreated in userland. This PR in particular can be recreated by using the example userland implementation of createRequireFunction and adding the following to the top of any module that wishes to have import.meta.require:

import Module from 'module';
function makeRequire(filepath) {
  const m = new Module(filepath, null);
  m._compile('module.exports=require', filepath);
  return m.exports;
}
// extract https://github.com/nodejs/node/blob/9deca876bb94626564184ad263ff80dc8042808b/lib/internal/url.js#L1395-L1403
// to get getPathFromURL
import.meta.require = makeRequire(getPathFromURL(import.meta.url));

As this comment consistently arises in a variety of location I'd like to delay both this PR and the one referenced until a quorum on what is required to justify implementation of new APIs into core related to these features. Both are able to be implemented purely in JS and core contributors consistently comment about how we should not add features that can be recreated using purely JS userland implementations.

benjamingr · 2018-06-19T10:32:34Z

@bmeck well:

@nodejs/modules I am not planning to land this without consensus from the group, but I think that this is a necessary base feature no matter what implementation we move forward with. I'll open a corresponding issue to discuss.

This will not land without further discussion anyway :)

bmeck · 2018-06-19T13:21:37Z

@benjamingr true, but that is a different reason than my current blocker.

benjamingr · 2018-06-19T14:13:10Z

@bmeck well, m._compile isn't public. Moreover, ESM is experimental which means it doesn't have the same standards requirements for API changes (those same checks are required - just later on when moving it to unflagged status).

bmeck · 2018-06-19T15:15:38Z

@benjamingr per my comment in #19360 (comment) , I note that while m._compile probably should not be seen as problematic in my implmentation. It was exposed publicly even if it isn not documented. It may not be intentionally an ecosystem concern, but the quick search around public source code shows it has decent usage in the wild (have to crawl through plenty of false positives though). Like many ecosystem concerns, it may not be possible to change/remove that method at this point.

The experimental status of ESM is unrelated to the ability to recreate this PR in JS which is the claim for why this PR and others cannot land. Even without regard of this being an addition to the ESM bootstrap, the addition of this is possible using JS alone unless we cannot access the CJS module system from ESM which seems problematic if we intend to use CJS within future applications. We can talk about the claim of features being implementable in JS as being a blocker and if that is a valid argument, but it is being used in multiple places by multiple people and I am using it here.

benjamingr · 2018-06-19T15:34:02Z

Right, but it's not using a public API (it's using m._compile) so regardless of whether or not "being implementable in userland JS" is a valid reason for landing or not landing a module - in this case this is not implementable in userland.

In fact - this PR is exactly exposing a capability that is not possible in userland JS without accessing internal APIs.

bmeck · 2018-06-19T15:41:47Z

@benjamingr we have public APIs using _ as a prefix, this API is not documented and is able to be gotten to without any sort of privilege required. I consider it public.

In fact - this PR is exactly exposing a capability that is not possible in userland JS without accessing internal APIs.

I disagree on m._compile being internal at this point. The CJS APIs are largely unable to be refactored due to ecosystem constraints. We should be treating them as public unless there is the possibility of changing them. Even if they are undocumented, they cannot feasibly be removed without larger ecosystem impact.

jkrems · 2018-06-19T15:59:38Z

I disagree on m._compile being internal at this point. The CJS APIs are largely unable to be refactored due to ecosystem constraints.

I think there's an important distinction to be made between "fragile implementation details that the ecosystem depends on" and "official public APIs". Because otherwise this just becomes a circular argument. Yes, we can't remove or rename m._compile even though it's clearly not a well-designed public API (anybody who ever used this workaround would hopefully agree). But the reason why we can't remove it isn't only that it would break userland - it's also that node doesn't offer an official alternative. Offering an official API for use cases that currently require the use of fragile APIs that were designed for internal use isn't on the same level as adding APIs that duplicate self-contained userland libraries.

If you are suggesting to make m._compile an official API and documenting it ("it's CJS, let's officially give up on offering good APIs for it"), that'd be something else. But if you do, we should be clear about it.

That being said - I can understand your frustration. The somewhat arbitrary use of "can be done in userland" without any clear rubric for what it actually means opens the door for people shutting down APIs they don't like or don't think are important because technically any API could be done in userland with enough imagination. Which is problematic and worth addressing imo.

bnoordhuis · 2018-06-19T20:29:41Z

The somewhat arbitrary use of "can be done in userland" without any clear rubric for what it actually means

I've outlined the basic rules in #19360 (comment). Seems clear enough to me but happy to take questions.

Fishrock123 · 2018-06-20T14:40:49Z

Correct me if I'm wrong but doesn't this introduce some load order problems in the load graph? @bmeck would probably be more in tune with the details than myself.

bmeck · 2018-06-20T14:51:49Z

@Fishrock123 you can only use require() after dependencies have been evaluated if that is what you mean. However, this PR does not remove the ability to have static CJS imports from ESM so you can preserve the ordering you want by using static imports. It is only a problem for load ordering if it removes the ability to statically import CJS.

Fishrock123 · 2018-06-20T16:13:46Z

I think we should kick this to the modules working group then since @MylesBorins has already expressed that is his goal for the future.

This adds require to the import.meta object This can be used as a mechanism to get interoperability between common.js and esm.

MylesBorins · 2018-07-09T17:36:31Z

rebased against master and added @zenparsing's fix to support node_modules

MylesBorins · 2018-08-22T22:34:41Z

Closed in favor of nodejs/ecmascript-modules#1

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jun 13, 2018

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 13, 2018

MylesBorins mentioned this pull request Jun 13, 2018

Pull request opened for import.meta.require on core nodejs/modules#130

Closed

devsnek reviewed Jun 13, 2018

View reviewed changes

mcollina approved these changes Jun 13, 2018

View reviewed changes

MylesBorins force-pushed the import-meta-require branch from 21b9753 to f236623 Compare June 13, 2018 19:38

jdalton approved these changes Jun 13, 2018

View reviewed changes

jkrems approved these changes Jun 13, 2018

View reviewed changes

guybedford requested changes Jun 13, 2018

View reviewed changes

jdalton reviewed Jun 13, 2018

View reviewed changes

bmeck suggested changes Jun 18, 2018

View reviewed changes

Trott mentioned this pull request Jun 19, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-06-20 nodejs/TSC#553

Closed

MylesBorins force-pushed the import-meta-require branch 2 times, most recently from b1d4068 to bf9a18c Compare June 28, 2018 06:36

MylesBorins mentioned this pull request Jun 28, 2018

Proposal: minimal esm implementation nodejs/modules#141

Closed

targos and others added 4 commits July 9, 2018 13:34

module: add import.meta.require

a6d78ed

This adds require to the import.meta object This can be used as a mechanism to get interoperability between common.js and esm.

test: add tests for import.meta.require

ab93018

doc: document import.meta.require

4ecebf4

module: Add node_modules paths lookup to import.meta.require

cd7191c

MylesBorins force-pushed the import-meta-require branch from bf9a18c to cd7191c Compare July 9, 2018 17:35

fixup: doc fix

44189dc

bmeck mentioned this pull request Aug 10, 2018

modules: remove file extension and directory resolving for esm #21729

Closed

MylesBorins mentioned this pull request Aug 22, 2018

Import meta require nodejs/ecmascript-modules#1

Closed

MylesBorins closed this Aug 22, 2018

Import.meta.require #21317

Import.meta.require #21317

Conversation

MylesBorins commented Jun 13, 2018

nodejs-github-bot commented Jun 13, 2018

MylesBorins commented Jun 13, 2018

MylesBorins commented Jun 13, 2018

ljharb commented Jun 13, 2018

MylesBorins commented Jun 13, 2018 • edited Loading

guybedford commented Jun 13, 2018 • edited Loading

ljharb commented Jun 13, 2018

devsnek Jun 13, 2018

Choose a reason for hiding this comment

MylesBorins Jun 13, 2018

Choose a reason for hiding this comment

devsnek Jun 13, 2018

Choose a reason for hiding this comment

MylesBorins Jun 13, 2018

Choose a reason for hiding this comment

devsnek Jun 13, 2018

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jun 13, 2018

bmeck commented Jun 13, 2018

ljharb commented Jun 13, 2018

jkrems commented Jun 13, 2018 • edited Loading

bmeck commented Jun 13, 2018

MylesBorins commented Jun 13, 2018

MylesBorins commented Jun 13, 2018 • edited Loading

MylesBorins commented Jun 13, 2018

jkrems left a comment

Choose a reason for hiding this comment

guybedford commented Jun 13, 2018

MylesBorins commented Jun 13, 2018

guybedford left a comment • edited Loading

Choose a reason for hiding this comment

jdalton Jun 13, 2018

Choose a reason for hiding this comment

bmeck Jun 13, 2018

Choose a reason for hiding this comment

jkrems Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Jun 13, 2018

bmeck commented Jun 13, 2018

jdalton commented Jun 13, 2018

bmeck left a comment

Choose a reason for hiding this comment

benjamingr commented Jun 19, 2018

bmeck commented Jun 19, 2018

benjamingr commented Jun 19, 2018

bmeck commented Jun 19, 2018

benjamingr commented Jun 19, 2018

bmeck commented Jun 19, 2018

jkrems commented Jun 19, 2018 • edited Loading

bnoordhuis commented Jun 19, 2018

Fishrock123 commented Jun 20, 2018

bmeck commented Jun 20, 2018

Fishrock123 commented Jun 20, 2018

MylesBorins commented Jul 9, 2018

MylesBorins commented Aug 22, 2018

MylesBorins commented Jun 13, 2018 •

edited

Loading

guybedford commented Jun 13, 2018 •

edited

Loading

jkrems commented Jun 13, 2018 •

edited

Loading

MylesBorins commented Jun 13, 2018 •

edited

Loading

guybedford left a comment •

edited

Loading

jkrems Jun 13, 2018 •

edited

Loading

jkrems commented Jun 19, 2018 •

edited

Loading