nodejs / node-eps

Node.js Enhancement Proposals for discussion on future API additions/changes to Node core
442 stars 66 forks source link

WRT PR #3 - ES module path resolution #11

Closed bmeck closed 7 years ago

bmeck commented 8 years ago

This is the place to discuss specifics of how the EcmaScript (ES) module imports will resolve paths. The proposal itself can be seen in PR #3.

Discussions here should regard:

It should not discuss import/export conversion, evaluation ordering, or detecting file mode.

caridy commented 8 years ago

To be clear, this is about import and export statements, whether they are declarative or imperative calls, it doesn't really matter because they are reflective somehow.

Additionally, we should, somehow, try to weight-in the resolution process used in browsers, to facilitate portability, I think @domenic can give us a quick overview of what we have so far in HTML.

Questions to be ask:

  1. should the importer always include the file extension?
  2. should we disallow the ambitious mechanism we do today in node to resolve index.js? specifically, the fact that require('./something') could resolve in ./something.js or ./something/index.js.
  3. should we allow introspection into the module's internals? e.g.: import foo from "mod/lib/util"; or should we consider this a legacy behavior, and if you want to access the internals of the module, you should use require(), while an npm pkg will have a single es6 module that represents its api? (this somehow leaks into the discussion about .jsm vs package.json out of band signal)
domenic commented 8 years ago

What browsers are implementing is documented at https://html.spec.whatwg.org/#resolve-a-module-specifier. The answer to your questions for browsers are:

  1. Yes, file extension is always necessary; import "./foo" looks for a file named foo, not one named foo.js.
  2. Yes, index.js automagic is disallowed. import "./something" looks for a file named something, not one named something/index.js.
  3. Not applicable yet, as any non-absolute-URL import specifiers not prefixed with /, ./, or ../ fail. Both seem possible in the future; when a mapping table API is created (manifest etc.) it could either be based on prefix-matching (allowing inspection of "internals") or just string equality (only allow inspection of "internals" if explicitly exposed in the manifest).
bmeck commented 8 years ago

The proposal currently has these specified. Currently the behavior is:

  1. yes, importer must include file extension
  2. this is disallowed if not looking for a package, but when searching for package main we use the current require algorithm.
  3. currently this is allowed, it is used by various packages on npm and would be a hard sell. Currently the proposal does require file extensions for inner files, that would only affect file extension (keep that discussion contained to https://github.com/nodejs/node-eps/issues/13).
caridy commented 8 years ago

@domenic yes, 3 does not apply to the browser, although you could enforce that in the future via loader hooks.

@bmeck excellent, 1 and 2 seems to be very straight forward. As for 3, I think the various packages on npm today that are doing so are fat packages, and the transpiled version of their source are doing require() anyways. Once node supports ES modules, they will have to rework their pkgs a little bit, and should be fine to introduce this restriction. For years, we have been battling this, it is a refactor hazard for module authors, and signaling that to consumers of the modules by forcing them to use an alternative api to import those internals, seems like a good tradeoff here.

bmeck commented 8 years ago

@caridy banning inner files will cause some issues, but can probably smooth that over. Going to say we ban inner imports for packages.

domenic commented 8 years ago

I think it would be good to give people an opt-in way to expose their internals. I agree it's a hazard that they're automatically exposed, but for e.g. packages like especially or IIUC lodash it's an important part of their API.

caridy commented 8 years ago

@caridy banning inner files will cause some issues, but can probably smooth that over. Going to say we ban inner imports for packages.

@bmeck I'm not proposing banning them, obivously, inside your pkg you can do whatever you want since you can do ./path/to/relative/files.js. My proposal is to ban it when using import "packageName/fileName.js";

packages like especially or IIUC lodash it's an important part of their API.

@domenic Yes, we have few popular pkgs using that technique, and those will continue to work for people who are using require() to import them, but if you want to use an import statement, you will have to do something like: import {Math} from "especially"; which is itself a namespace produce by: export * as Math from './math.js'; in your index.js which will describe the API of the pkg.

jkrems commented 8 years ago

@caridy We have packages that include service clients and stubs to use for testing. We really don't want to load the service stubbing code into our production apps. Disallowing separate entry points sounds like a pretty big restriction. "You can keep using require" is another way of saying "if you want consistency in your code base and you're doing anything beyond toy examples, ignore import". Which would be a rather unfortunate outcome.

bmeck commented 8 years ago

@domenic would the reexport work if the loader does not evaluate any dependency that only has reexported bindings being imported (words @_@)?

// especially
export * as Math from './math.js';
// consumer
import {Math} from 'especially';

If we specify that especially does not evaluate since it has no direct bindings requested by consumer. Would that work? (me is rereading 262 since this would be a change it looks like, feel free to correct me).

domenic commented 8 years ago

I don't think there's any need to change the execution strategy. Reexporting would work I guess. Just kind of ugly.

bmeck commented 8 years ago

Forget what I said, side effects could mean the execution order needs preservation. We can't skip.

caridy commented 8 years ago

@caridy We have packages that include service clients and stubs to use for testing. We really don't want to load the service stubbing code into our production apps. Disallowing separate entry points sounds like a pretty big restriction.

@jkrems this is precisely what this discussion is for, to surface use cases, like yours. Can you elaborate more about the structure of your pkg? maybe a link that we can look at?

"You can keep using require" is another way of saying "if you want consistency in your code base and you're doing anything what toy examples, ignore import". Which would be a rather unfortunate outcome.

to reiterate, it is the consumer or existing pkgs using require() that will continue using require() for their convenience. If, as a consumer, you really want to use import, then you follow the new API provided by the author of the pkg.

timoxley commented 8 years ago

We have packages that include service clients and stubs to use for testing. We really don't want to load the service stubbing code into our production apps.

This is a use case I've hit and pondered with ES6 modules a few times. While it's possible to control what gets exported, there doesn't seem to be any way to conditionally prevent unneeded code from being loaded at all.

bmeck commented 8 years ago

@timoxley you will need to use an imperative function to load things conditionally:

// declare what can be exported:
export if_debug;

// conditionally load using require
let if_debug = IS_DEBUG ? require('foo') : null;

// conditionally load using System.loader.import
let if_debug = null;

// block completion using `await` , this would not affect require
// after this point if_debug is defined
await System.loader.import('foo').then(ns => if_debug = ns;)
caridy commented 8 years ago

additionally, once we settle on the api for the import() imperative api, you should be able to do something like:

export let if_debug = null;

if (something === true) {
    if_debug = await import('./something');
}

and that is not that far from what you do today in node.

ljharb commented 8 years ago

fwiw, "optional file extensions" and "index.js automagic" is something that's pretty easy to implement with rewrite rules on webservers, and I suspect that many will choose to do this so that require('./foo') in the browser works the same as node.

bmeck commented 8 years ago

@ljharb the requirement for file extension is to remove magic / cause less stat() calls in node. But yes, it is pretty easy to have rewrite rules for the server/browser layer.

domenic commented 8 years ago

Doesn't work on gh-pages.

timoxley commented 8 years ago

@bmeck dropping down to require is how I've handled conditional loading currently (via babel), but this unfortunately exposes ES6/commonjs interop complexities.

I support ES6 modules but that you can't do conditional loading without using require could be seen as an argument that we're upgrading to something worse/unnecessarily complicated – now we have import & export syntaxes, require and System.loader.import, when before we only had require and module.exports.

Also like to add I like the idea of decoupling the external interface from the internal file structure… currently internal file structure becomes a part of the public interface, so making any sort of file structure changes is potential API breakage, which is annoying and error prone for both producers and consumers.

timoxley commented 8 years ago

@bmeck will there be any require.extensions equivalent for hooking extra logic into the loader? or is this handled by interfering with System.loader?

bmeck commented 8 years ago

@timoxley we do not endorse require.extensions but under the current proposal it is honored. The WHATWG Loader does have hooks, but it is not specified that we will be using those since we have a different pipeline.

caridy commented 8 years ago

@timoxley read above to see how to load something conditionally without too much hazard. And no, you will have import, import() and export that should be more than enough.

ljharb commented 8 years ago

@domenic it wouldn't work as-is, but Github could easily add the simple rewrite rules required (or statically duplicate the relevant JS files in advance), so it's not outside the realm of possibility that it could work fine in the future.

caridy commented 8 years ago

@ljharb if we disallow the ability to do import "./foo" because it is ambiguous (it could be ./foo/index.js or ./foo.js), what's the point of omitting the extension?

As for the rewrite rules: just because every server out there can add a rewrite rule to align with node resolution logic doesn't mean they will do so. If we want to bring the npm resolution process to the front, small changes will have to be made for the greater good.

bmeck commented 8 years ago

@caridy we cannot place the burden completely on node to support all existing workflows (such as servers that will not change), we must balance the risks and best path for node here.

jokeyrhyme commented 8 years ago

One good thing about having such strict resolution is that we could start writing our code this way now, and it'll work in both new and old Nodes. We could also start writing new code (or porting old code) to avoid the need for deep imports (e.g. mymodule/something/within.js'. If this is the best advice, then we should begin to circulate it as soon as possible.

caridy commented 8 years ago

@jokeyrhyme :+1:

bmeck commented 8 years ago

Update, this is on tract to be accepted as Draft. I have received renewed argument about the removal of search paths via IRC.

In the PR @domenic was requesting the file extensions be mandatory. There was a concern over urls mapping to actual files.

In this thread @ljharb raised the point of the migration not being seamless because of removing the path searching. Talking on IRC, he is suggesting index.js and file extension searches be included in module implementation. Removal of package.json search outside of node module would still be excluded from import.

As this is a Draft, and as it is not Accepted, the community and ecosystem can still weigh in. Since path searching can be added and would not be conflicting with the current proposal, arguments can continue without preventing the existing proposal.

domenic commented 8 years ago

I see the following benefits to the current proposal's simpler search semantics:

ljharb commented 8 years ago

fwiw I'm fully in support of all the removals, except for a) auto-"index" when the path is a dir, and b) "package.json" detection (i want the first much more than the second).

My concerns over omitting auto-extension include that the vast majority of existing babel code omits the extension (it's often encouraged to omit it as a best practice), but most importantly, it means i can't import a module without knowing its format - ie, I have to know if it's CJS or not. It's hugely unergonomic in a codebase which is gradually migrating from CJS to babel-ES6 to native-ES6 to have to know, with certainty, which module format I'm importing. This was one of the major concerns during the whole extension vs parsing vs etc debate, and the current proposal discards the crucial symmetry of never having to know the module format of the thing you're importing.

domenic commented 8 years ago

, it means i can't import a module without knowing its format

That's just not true. If you use .js, it will work no matter if it's CJS or ES6.

domenic commented 8 years ago

Oh, right, nevermind, CJS = .js, ES6 = .mjs. So yeah, you need to know the filename of the thing you're importing---knowing the filename-minus-the-extension isn't sufficient, so be sure to turn on "show file extensions" in your Windows Explorer. Which I assume is what you're using, because no other modern software hides file extensions ;)

bmeck commented 8 years ago

My concerns are in both directions of browser and node:

  1. Static file servers such as http-server being able to serve <script type=module ... with minimal effort
  2. The breakage of existing babel code that relies on this module resolution behavior.

The current proposal is compatible with the script tag with base being the root of the file system. The only exception is for "bare" urls to my knowledge which are resolved via node_modules.

Knowledge of the file extension is somewhat separate as this was the case even prior to proposing a new file extension.

The idea of removing one of the bottlenecks for startup and compatibility with browsers is what we chose. However, the breakage of existing ecosystem can be somewhat mitigated by the errors showing the path problem.

Perhaps talking to toolchains implementing non-compatible resolution during transpilation so that they send out warnings would be sufficient.

zenparsing commented 8 years ago

I strongly recommend prioritizing symmetry between browser and node on this point. Backlash is inevitable given the current "best practices" (which are really just social norms in this case). Users can adapt quite easily.

ljharb commented 8 years ago

Users won't adapt unless the benefits of switching to ES6 modules are greater than the pains of doing so.

Allowing extensionless import resolution won't prevent anyone from using the extension. It will mean that people who want to continue omitting the extension can continue to do so - and they can continue to do so via server rewrite rules if they wish.

jokeyrhyme commented 8 years ago

Developers who want to leave out the extension and rely on magic resolution may continue to use require() directly, or transpilers that convert import to require(). Anyone already using import with magic resolution today is already using a transpiler.

The friction will occur when such a developer tries to remove their transpiler in favour of the native import. And if the current require() exceptions are anything to go by, it'll be pretty easy to understand what needs to be altered in order to use the new native import.

zenparsing commented 8 years ago

Users won't adapt unless the benefits of switching to ES6 modules are greater than the pains of doing so.

I simply don't buy the argument that developers will reject engine supported ES modules if they have to include an extension.

mariusGundersen commented 8 years ago

I want to suggest good support for package relative paths, which is missing from node today. Today all modules have to be specified using a relative path (starting with "./" or "../"), but this is not very common in other languages. It becomes a problem when moving a file, as you now have to edit the imports in that file, since the relative paths have changed.

In a large node package or application it will be useful to import modules relative to the root of the package, not the file system. In fact, I believe it is a lot more common to want to load a module relative to the root of the package than to the root of the file system, and so the former should be easier than the latter.

For a while I thought of making the suggestion that import x from "/something" would resolve relative to the package root, while import x from "file:///something" would resolve it absolutely on the file system (this way you could use http:// to load it over the network too). But after thinking about it some more I didn't find this to be good enough.

I still think there needs to be some simple way to specify a module relative to the package root (which is the first ancestor directory with a package.json, for example), but I don't have a good solution for how to do this. So I bring only a problem and my opinion, not a solution.

bmeck commented 8 years ago

@mariusGundersen please use something like npm link or linklocal to accomplish this. We do a similar action to this when explaining non-local dependencies, but the resolution algorithm already supports such behavior via symlinks.

Simply place a symlink in node_modules/name_of_root then perform

import "name_of_root/inside_root";
mariusGundersen commented 8 years ago

Sorry, I may not have been entirely clear. What I mean is that I want to import a module in the same package/project without using a path relative to the current file/module. If the project/package has several folders, then it would be nice to import a file/module in a subdirectory. Think of it like how C#, Java, Python, etc do namespaces/packages.

Currently you need to know where the module you want to import is relative to the module you want to import it into. For example, in controllers/myController.js I want to import the model in models/myModel.js. Today I have to use import myModel from '../models/myModel.js'. Most other languages let you do this without having to travers backwards. In other words, there is a way to reference the module/file from the project/namespace/package root: import myModel from '#models/myModel.js (I'm using # here as the standard symbol for proposed-functionality).

bmeck commented 8 years ago

@mariusGundersen create a symlink node_modules/models that points to ../models, use require or import to get models/myModel.js

A console dump of doing this in a clean directory:

LMDV-BRADLEY:link-example bfarias$ mkdir models
LMDV-BRADLEY:link-example bfarias$ echo 'console.log("model!")' > models/index.js
LMDV-BRADLEY:link-example bfarias$ mkdir node_modules
LMDV-BRADLEY:link-example bfarias$ ln -s ../models node_modules/models
LMDV-BRADLEY:link-example bfarias$ node
> require('models')
model!
{}
>
(To exit, press ^C again or type .exit)
>
LMDV-BRADLEY:link-example bfarias$ ls */
models/:
index.js

node_modules/:
models
LMDV-BRADLEY:link-example bfarias$ ls -l */
models/:
total 8
-rw-r--r--  1 bfarias  386085923  22 Apr 25 07:08 index.js

node_modules/:
total 8
lrwxr-xr-x  1 bfarias  386085923  9 Apr 25 07:09 models -> ../models
mariusGundersen commented 8 years ago

Right, now I get you :)

That works, but doesn't it feel quite hacky to you? Wouldn't it be better if this had built in support in the path resolving algorithm? The fact that we have to resolve to something like this indicates that there is a need for what I described above that is simpler and works with published packages too (Your method with symlink won't work if I publish that project on npm, right?).

bmeck commented 8 years ago

@mariusGundersen it is supported in the path resolution algorithm, I am unsure what needs to be added?

linklocal has the tooling to reconstruct the same symlinks upon installation as the example I showed you if you want to publish your package to npm (on the other side if npm was able to package up symlinks like a normal tar that wouldn't be an issue [windows has strange behavior preventing this]). I don't think we need to have multiple resolution mechanisms for bare import/require paths to do this since it is already simple to explain/some tools exist.

New resolution mechanics would need to bring more than what currently exists to the table. We will not be doing network based loading at this time for sure.

mariusGundersen commented 8 years ago

I guess we disagree on whether or not something that I'd a common problem needs a simple, built inn solution or if it is OK that it has a workaround only. Sure, using symlinks works now, but wouldn't it be better to support this without symlinks? I see a few problems with it, for example having a directory with the same name as an installed package (they would have the same name inside node_modules).

Wasn't it also discussed not to be able to import other files from a package using import? As in import x from 'lodash/core' wouldn't work anymore. Wouldn't that break the symlink workaround?

Isn't this a good opportunity to add a resolution mechanism that people have wanted for a long time? It could probably be added later, but since we have breaking changes anyway, this feels like a good time to introduce it.

jkrems commented 8 years ago

@mariusGundersen I actually would argue that such a resolution mechanism ("relative to root of project") is a net-negative because it encourages having spiderweb dependency graphs (everything depends on something else in a completely different part of the project) instead of a clean dependency tree. Adding a cost to reaching into different parts of the code base is a good thing imo. That's highly subjective of course, but it definitely isn't as simple as "this is clearly a missing feature that everyone is waiting for". :)

Can you elaborate more about the structure of your pkg? maybe a link that we can look at?

I don't think we have anything using that pattern on public Github. But the general idea is that a package contains both the actual library code and "test support code". The two are a) relatively small and b) tightly coupled. Which makes it awkward to split them into separate packages. Pseudo-code:

// In "production"/app code:
var MyClient = require('the-client-library');
new myClient(config.myClient);

// Somewhere in the test setup:
var fakeApi = express();
fakeApi.use(require('the-client-library/mocks'));
fakeApi.listen(config.myClient.port);
bmeck commented 8 years ago

@mariusGundersen the proposal still allows inner path resolution. The idea of removing it was discussed, but is not in the proposal. Please see:

Symlinks of [...] node_modules/foo/ -> $HOME/.node_modules/foo/, etc. will continue to be supported. [1]

import x from 'lodash/core' is one of the reasons the proposal is this way as noted in [2]

The solution is not a workaround, it is how path resolution works. Having a single flat namespace for "bare" path resolution is important. You can suggest tooling or proposals to encourage this but it should be using a different syntax that does not break backwards compatibility and is not related to this proposal.

gulbanana commented 8 years ago

Static file servers such as http-server being able to serve <script type=module ... with minimal effort

It's not going to be possible for static file servers to serve node .mjs files for <script, since they won't have the right mime type. So there will have to be some sort of transformation step anyway.

bmeck commented 8 years ago

@gulbanana browsers don't check for file mode via extension, also see note about mime, we can register the file extension to match the MIME if we start implementation for sure.

ljharb commented 8 years ago

@bmeck fwiw, import x from 'no_package_json_but_in_node_modules/foo' won't work with the current proposal, and does work with require :-(

Specifically, this is how many rails apps symlink app/assets/javascripts into node_modules so that they can avoid relative requires.

gulbanana commented 8 years ago

If IANA accepts .mjs as an alternative extension for application/javascript then it would (eventually) solve the problem, yeah.