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

MylesBorins · 2018-07-09T21:58:29Z

this build on top of the limitations of the package name map proposal.
It removes file extensions and directory resolution in the resolver.

This is only currently limited for local development. When resolving
dependencies file extension and directory resolution will still work.

Refs: https://github.com/domenic/package-name-maps

Alternative to #21572

nodejs-github-bot · 2018-07-09T21:58:30Z

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

MylesBorins · 2018-07-09T21:59:34Z

ping @zenparsing I borrowed ResolvesToFile from your branch. LMK how you would like to be attributed

95aad4f#diff-4a5950be44f56dfca2e7fa3c4e1fc0e4R551

MylesBorins · 2018-07-09T22:02:42Z

src/module_wrap.cc

@@ -632,10 +638,27 @@ Maybe<URL> ResolveDirectory(Environment* env,

 }  // anonymous namespace

-Maybe<URL> Resolve(Environment* env,
+Maybe<URL> ResolveRecurse(Environment* env,


This was done to make a separate code path for resolving dependencies than local modules. Not 100% happy with the current implementation... but there is definitely a difference in the algorithm now (still need to resolve main for modules).

Open to suggestions on better ways to do this

If this is renamed to ResolveRelativeWithExtensions, and used only by main lookups, then the ShouldBeTreatedAsRelativeOrAbsolutePath check can be skipped.

bmeck

I'm against this while we continue discussions in the Modules WG and am in fact increasingly concerned about how package-name-maps work and the viability and usability of matching them so going to wait on those discussions.

MylesBorins · 2018-07-09T22:13:06Z

@bmeck totally reasonable. I look forward to discussing it. To be explicit I consider this a WIP, and have added the appropriate labels.

bmeck · 2018-07-09T22:15:05Z

@MylesBorins to be clear I am fairly -1 to package-name-map parity currently and will probably be needing more than just technical work to not think this is a bad idea.

MylesBorins · 2018-07-09T22:17:05Z

@bmeck makes sense to discuss in the WG.

guybedford · 2018-07-11T20:25:58Z

I am +1 to this.

guybedford

A few comments on simplifying the implementation, but otherwise seems good to go for experimental to me.

guybedford · 2018-07-11T20:34:21Z

src/module_wrap.cc

@@ -586,9 +592,9 @@ Maybe<URL> ResolveMain(Environment* env, const URL& search) {
    return Nothing<URL>();
  }
  if (!ShouldBeTreatedAsRelativeOrAbsolutePath(pjson.main)) {
-    return Resolve(env, "./" + pjson.main, search, IgnoreMain);
+    return ResolveRecurse(env, "./" + pjson.main, search, IgnoreMain);


This can probably be simplified to a ResolveRelativeWithExtensions, that doesn't need to do the absolute checks and assumes its input is a relative URL.

guybedford · 2018-07-11T20:37:21Z

src/module_wrap.cc

@@ -599,7 +605,7 @@ Maybe<URL> ResolveModule(Environment* env,
  do {
    dir = parent;
    Maybe<URL> check =
-        Resolve(env, "./node_modules/" + specifier, dir, CheckMain);
+        ResolveRecurse(env, "./node_modules/" + specifier, dir, CheckMain);


For simplicity, this can stay as just Resolve.

guybedford · 2018-07-11T20:38:08Z

src/module_wrap.cc

@@ -632,10 +638,27 @@ Maybe<URL> ResolveDirectory(Environment* env,

 }  // anonymous namespace

-Maybe<URL> Resolve(Environment* env,
+Maybe<URL> ResolveRecurse(Environment* env,


If this is renamed to ResolveRelativeWithExtensions, and used only by main lookups, then the ShouldBeTreatedAsRelativeOrAbsolutePath check can be skipped.

guybedford · 2018-07-11T20:38:36Z

src/module_wrap.cc

-    }
-    return ResolveDirectory(env, resolved, check_pjson_main);
+    if (ResolvesToFile(resolved))
+      return Just(resolved);


Beautiful :)

That's all @zenparsing

BTW, this C++ code is a pleasure to work with 👍

All the implementation work of @bmeck, with many various refactoring contribs.

MylesBorins · 2018-07-11T21:06:57Z

@guybedford wanna push a patch to address your comments? I'll be afk the rest of the day and likely not have a tonight time to work on this this week

guybedford · 2018-07-11T22:49:31Z

@MylesBorins also playing holiday catch-up here, but will add it to the list.

guybedford · 2018-07-15T11:02:25Z

I've added the changes here, although the latest commit on this branch with some typo corrections isn't being displayed by GitHub for me for some reason... MylesBorins@be9afef.

MylesBorins · 2018-08-09T20:07:08Z

@nodejs/modules @bmeck is this perhaps something we can move forward with if we get all the tests passing?

mcollina · 2018-08-09T20:10:06Z

I’m +1 in getting this landed.

this build on top of the limitations of the package name map proposal. It removes file extensions and directory resolution in the resolver. This is only currently limited for local development. When resolving dependencies file extension and directory resolution will still work. Refs: https://github.com/domenic/package-name-maps

bmeck · 2018-08-10T12:00:31Z

but the window of opportunity is closing to even have the ability to consider supportting this unless we move on this PR soon.

We cannot saddle both sides of "node's implementation is experimental and subject to breaking changes" and "node's implementation is experimental and enforces patterns". My fear here is we are making a big change to the implementation with great impact on a variety of topics without discussion of those.

I don't even see mention on migration patterns for multi-mode packages here? Where is the discussion about the impacts of landing this. Similar to #21317 I don't think this PR is looking at overall impact but instead is scoping its focus to finely to a specific change that makes us lose sight of what this would do to an overall implementation.

I also see an incomplete and what I see as problematic package-name-map spec as a large part of the reasoning here, but as pointed out in the past you can achieve this kind of "searching" in package name maps so that invalidates this line of reasoning entirely. I think this needs to be more well discussed about impact than just saying it is for some level of compatibility with a potential future web spec that contradicts this PR.

I do consider this change controversial and do not want to see it being discussed without recognizing the impact that this has. Right now it is being phrased as:

a simple change but actually has major affects on migration.
a web compatibility change but actually is contradicted by the early stage spec it is saying it needs the change for
a reasonable reservation of future design space but isn't recognizing the ecosystem breakage and the feedback from a minimal implementation PR that tried this

demurgos · 2018-08-10T12:13:32Z

@bmeck

restricting to mandatory extensions seems safe if the goal is to restrict Node's current ESM support to a common core shared by all proposals.

I actually find this to be a breaking change that should not be considered part of a common core. For reasons like you lay out in nodejs/modules#139 ; it has interoperability problems not just with existing code, but also with migrations.

By "common core shared by all proposals", I specifically mean that it does not rule out any proposal. Optional extensions can be restored later, but having optional extensions prevents any proposal where they are mandatory.

As I stated above and in my interop issue, I think that mandatory extensions would cause a migration failure and I am strongly opposed to them in the final product. It's just that until there is consensus, mandatory extensions are the less-commiting choice (you can't remove optional extensions once they are out). I consider landing this PR would be q step backward, but I understand why it's pushed by some people, and it does not preclude reintroducing optional extensions later on. I have invested time in tools relying on the current behavior, but I am fully aware that the implementation is experimental and the burden is on me to keep up with the changes.

Edit: Essentially, my comment above corresponds to your third point:

a reasonable reservation of future design space but isn't recognizing the ecosystem breakage and the feedback from a minimal implementation PR that tried this

I am mostly worried because I did not expect the current implementation to change until the discussions were resolved.

guybedford · 2018-08-10T12:58:25Z

We cannot saddle both sides of "node's implementation is experimental and subject to breaking changes" and "node's implementation is experimental and enforces patterns".

We kind of have to accept both to some degree. Anything that works in the experimental implementation will be a painful break at some level if it does not work in the non-experimental implementation. Eg for this reason it is not possible to remove ".mjs" at this point - it is already cemented into the ecosystem (not that we would of course).

My fear here is we are making a big change to the implementation with great impact on a variety of topics without discussion of those.

Agreed, let's discuss.

I don't even see mention on migration patterns for multi-mode packages here? Where is the discussion about the impacts of landing this.

Multi-mode support would still work for consumer-side import 'pkg/x' and import 'pkg', but not for import '/path/to/pkg/x'.

This change of no longer supporting "importing a folder being equivalent to importing a package" is a very important concern definitely, and the loss of a use case in Node.

We could consider a change to this PR to retain the "treat folders as packages" behaviour, while still removing the automat extensions behaviours.

@MylesBorins let me know if you could get behind such a change as well.

@bmeck do please share any other concerns on this.

devsnek

this change breaks the world for people like me who are working on developing the loader. additionally to this we agreed on a moratorium which was in part your own suggestion...

guybedford · 2018-08-10T13:36:34Z

this change breaks the world for people like me who are working on developing the loader.

@devsnek can you perhaps share some details on how this affects things for you?

devsnek · 2018-08-10T13:39:59Z

@guybedford it breaks things because node is barely usable without node-style resolution. even if this is undone at some point in the future I see no reason to kneecap ourselves.

guybedford · 2018-08-10T13:44:10Z

it breaks things because node is barely usable without node-style resolution. even if this is undone at some point in the future I see no reason to kneecap ourselves.

@devsnek could you give an example of what you would expect that wouldn't work here that you deem as this kind of compatibility loss based on my amended proposal in #21729 (comment)?

bmeck · 2018-08-10T13:46:01Z

This change of no longer supporting "importing a folder being equivalent to importing a package" is a very important concern definitely, and the loss of a use case in Node.

I'd say it is more impactful than a mere single use case. The divergence in behaviors means things like resolve('pkg/foo') is unable to be used as a cached value if the use of it would eventually rely on searching:

const foo = await resolve('pkg/foo');
import(new URL('./bar', foo));

would fail and need to be rewritten as:

 // this should be rewritten in a safe way to compose specifier paths
// I'm unclear on how to do this easily right now
const foo = 'pkg/foo';
import(`${foo}/../bar`);

but the above actually has a bug in it compared to the original. It imports pkg/bar which may not be the same thing that pkg/foo pointed to when resolved. For example, pkg/foo could have a package.json pointing to pkg/foo/lib/index.js, leading the original to end up with ~ pkg/foo/lib/bar and the rewritten to be pkg/bar.

guybedford · 2018-08-10T13:52:01Z

@bmeck my amendment in #21729 (comment) would deal with that case as discussed in that comment, so that import(new URL('./bar', foo)); would still resolve correctly.

bmeck · 2018-08-10T13:53:13Z

@guybedford new URL('./bar', foo) resolves to an absolute URL which seems exactly what you call out as not working:

not for import '/path/to/pkg/x'.

devsnek · 2018-08-10T13:56:59Z

@guybedford when I resolve ./x I expect to not have to know how x is implemented. if x is 500 files in a directory or a single .json file or whatever doesn't matter. I'm not really sure why this isn't considered like the bare minimum here.

guybedford · 2018-08-10T13:59:59Z

@bmeck I'm referring to:

We could consider a change to this PR to retain the "treat folders as packages" behaviour, while still removing the automatic extensions behaviours.

To try and clarify the approach, we could do a package-boundary check, and then apply extensions only if the parent package boundary is a different package to the resolved package boundary.

GeoffreyBooth · 2018-08-10T14:08:23Z

We agreed at the last meeting to work on creating a minimal implementation that could be a core that various implementations could build on. It seems to me that what's in core Node should be that minimal implementation, even if it's not fully usable and doesn't satisfy most use cases. This PR seems to me to be along those lines. I think it's time for the "full" experimental-modules implementation to live in a branch, and what's in core is the minimal base we agreed to build.

Agreed that there are some issues with this approach causing differences when loading with a full path or reative path to other packages, so dismissing my review. I think we may still be able to make some progress on this using package boundaries as discussed in #21729, and I think the potential perf benefits for Node would be worth continuing to explore this.

MylesBorins · 2018-08-10T16:22:43Z

re: moratorium

It was my understanding that we are indeed blocked on landing such changes without consensus within the team, but I did not believe that the moratorium was in place to block opening PRs as an attempt to explore a problem space.

If individuals in the team find these PRs to be problematic I am more than happy to not continue this method of exploring a problem.

GeoffreyBooth · 2018-08-10T18:46:36Z

The moratorium shouldn’t extend to PRs. If it does we’ll never make any progress.

devsnek · 2018-08-10T18:50:34Z

i'm used to prs meaning someone wants to land something. if that wasn't the intention here then i suppose its fine... a label or title change might not go amiss to make sure people understand it.

bmeck · 2018-08-10T19:24:48Z

@GeoffreyBooth the moratorium was on landing code. PRs are generally used to land code.

That said, I think as long as we get full consensus we can land "non-controversial" PRs. I actually think that for things to be non-controversial to myself at least they need to discuss the interwoven impact they have on other parts of the implementation. It would probably be better to do them as a combined PR for example in the case of this and import.meta.require/createRequireFunction since it affects migration and at the last meeting we talked about potentially not including the ability to import .js files at all. Doing them piece by piece like this prevents us from getting a scope of direction, and could lead to situations where we land a PR then have to revert it because of a different concern.

For example if we were to split up the PRs for:

remove path searching
remove ability to import .js files entirely
exposing createRequireFunction/import.meta.require

If we land any of those, the others have a problem because they are interwoven in use cases.

If we were to remove path searching only, we now encourage everyone to import .js files to load CJS. This is what some people see as an undesirable path forward since they wish to use .js for ESM by default.

If we were to remove the ability to import .js file entirely only, then we no longer have a way to load CJS.

If we were to export some level of require only, we would be encouraging people to use CJS APIs rather than import()/import ... from .../export ... from .... Which is problematic because a lot of people want all new code to be written in ESM viable APIs or because people can't use those APIs on the web.

We can combine these PRs to make a full story on a minimal kernel of sorts, but it doesn't make sense to land them separately without having that story in place.

GeoffreyBooth · 2018-08-10T19:31:03Z

it doesn't make sense to land them separately without having that story in place.

Perhaps we could make these PRs against some other branch? To keep the PRs minimal but assemble the story elsewhere until it's cohesive and could then be merged into core.

bmeck · 2018-08-10T19:34:14Z

@GeoffreyBooth that seems ok. Would put them all to land on that fork/branch and have the overarching discussions before coming back here I would assume. Getting to consensus would still take time as long as that is ok.

benjamingr

I don't think this should land without a modules team meeting but I'm +1 on the actual change and explicitness.

robpalme

Like @benjamingr I think the change embodies the direction we agreed in the previous meeting and we should discuss this in the next meeting before landing.

ljharb

Looking forward to discussing in a meeting; i find extension and directory resolution a critical feature to have by default, so i don’t think this should ever land.

benjamingr · 2018-08-11T16:41:39Z

Looking forward to discussing in a meeting; i find extension and directory resolution a critical feature to have by default

@ljharb would you mind elaborating on why?

ljharb · 2018-08-11T17:26:10Z

The entire npm ecosystem, including best practices around specifiers, relies on full resolution. Package name maps are hopefully being added to browsers to make them more like node - it would be a massive step backwards for node to lose this feature.

MylesBorins · 2018-08-22T22:37:06Z

Moved to nodejs/ecmascript-modules#2

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jul 9, 2018

MylesBorins mentioned this pull request Jul 9, 2018

modules: enforce extensions and dissalow directories #21572

Closed

MylesBorins commented Jul 9, 2018

View reviewed changes

MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Jul 9, 2018

bmeck suggested changes Jul 9, 2018

View reviewed changes

MylesBorins mentioned this pull request Jul 9, 2018

Proposal: minimal esm implementation nodejs/modules#141

Closed

MylesBorins added the wip Issues and PRs that are still a work in progress. label Jul 9, 2018

MylesBorins changed the title ~~modules: remove file extension and directory resolving for esm~~ [WIP] modules: remove file extension and directory resolving for esm Jul 9, 2018

MylesBorins mentioned this pull request Jul 9, 2018

import-maps a proposal for bare imports in browsers nodejs/modules#51

Closed

guybedford previously approved these changes Jul 11, 2018

View reviewed changes

guybedford force-pushed the simplified-esm-resolver branch 2 times, most recently from acf6ff3 to 177910d Compare July 12, 2018 08:55

guybedford force-pushed the simplified-esm-resolver branch from 2c75c3f to 177910d Compare July 15, 2018 10:57

MylesBorins and others added 4 commits August 9, 2018 16:13

refactor ResolveRecurse into ResolveMain

cc6140e

build fixes

ba1aefd

fixup: more build fixes

3e16cf5

MylesBorins force-pushed the simplified-esm-resolver branch from be9afef to 3e16cf5 Compare August 9, 2018 20:30

devsnek suggested changes Aug 10, 2018

View reviewed changes

benjamingr approved these changes Aug 11, 2018

View reviewed changes

robpalme approved these changes Aug 11, 2018

View reviewed changes

ljharb suggested changes Aug 11, 2018

View reviewed changes

MylesBorins mentioned this pull request Aug 15, 2018

Managing fork and agreed upon minimal kernel nodejs/modules#166

Closed

MylesBorins mentioned this pull request Aug 22, 2018

modules: remove file extension and directory resolving for esm nodejs/ecmascript-modules#2

Closed

4 tasks

MylesBorins closed this Aug 22, 2018

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

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

Conversation

MylesBorins commented Jul 9, 2018

nodejs-github-bot commented Jul 9, 2018

MylesBorins commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck left a comment

Choose a reason for hiding this comment

MylesBorins commented Jul 9, 2018

bmeck commented Jul 9, 2018

MylesBorins commented Jul 9, 2018

guybedford commented Jul 11, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Jul 11, 2018

guybedford commented Jul 11, 2018

guybedford commented Jul 15, 2018 • edited Loading

MylesBorins commented Aug 9, 2018

mcollina commented Aug 9, 2018

bmeck commented Aug 10, 2018

demurgos commented Aug 10, 2018 • edited Loading

guybedford commented Aug 10, 2018

devsnek left a comment

Choose a reason for hiding this comment

guybedford commented Aug 10, 2018

devsnek commented Aug 10, 2018

guybedford commented Aug 10, 2018

bmeck commented Aug 10, 2018 • edited Loading

guybedford commented Aug 10, 2018

bmeck commented Aug 10, 2018 • edited Loading

devsnek commented Aug 10, 2018

guybedford commented Aug 10, 2018

GeoffreyBooth commented Aug 10, 2018

MylesBorins commented Aug 10, 2018 • edited Loading

GeoffreyBooth commented Aug 10, 2018

devsnek commented Aug 10, 2018

bmeck commented Aug 10, 2018 • edited Loading

GeoffreyBooth commented Aug 10, 2018

bmeck commented Aug 10, 2018

benjamingr left a comment

Choose a reason for hiding this comment

robpalme left a comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

benjamingr commented Aug 11, 2018

ljharb commented Aug 11, 2018

MylesBorins commented Aug 22, 2018

guybedford commented Jul 15, 2018 •

edited

Loading

demurgos commented Aug 10, 2018 •

edited

Loading

bmeck commented Aug 10, 2018 •

edited

Loading

bmeck commented Aug 10, 2018 •

edited

Loading

MylesBorins commented Aug 10, 2018 •

edited

Loading

bmeck commented Aug 10, 2018 •

edited

Loading