nodejs / node-eps

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

WRT PR #3 - Import relative to package root #21

Closed mariusGundersen closed 8 years ago

mariusGundersen commented 8 years ago

The current proposal adds support for the following path resolutions:

But what's missing from this list is importing relative to the current project (package) root. This is quite common in other languages, and other languages even prefer absolute imports and discourage relative imports. It's therefore unfortunate that in node we don't have the option or opportunity to decide for ourselves on a per-project basis.

The semantics are pretty straight forward: traverse the directory tree upwards from the current file looking for a folder that contains a file with the name package.json, and use this folder as root. This is pretty similar to the way the node_modules folder is found, so won't add any performance issues.

My suggestion for the syntax is to use the /something rule, and to use file:///something for absolute imports from the filesystem root. A rule of thumb in api design is that common problems should have simple solutions and uncommon problems should have solutions. I believe that importing modules from the filesystem root is uncommon, while importing modules from the package root is common, and therefore the latter should have a simpler solution than the former.

This doesn't make anything that is possible today impossible, and doesn't make it significantly more difficult to transpile or to convert CJS modules to ES modules.

calebmer commented 8 years ago

+1 from me. This would also probably discourage people from using NODE_PATH for this. The syntax I've seen in some configurations is:

import x from '~/something.js'
feross commented 8 years ago

-1. This feels like feature creep. The module system should not change at this point because it is "Stability: 3 - Locked".

Stability: 3 - Locked
Only fixes related to security, performance, or bug fixes will be accepted.
Please do not suggest API changes in this area; they will be refused.

See https://nodejs.org/api/documentation.html

calebmer commented 8 years ago

If we are considering changing module resolution with ES modules, this feature might be worth considering.

mariusGundersen commented 8 years ago

@feross, this is only for ES6 modules using import, so should fall under the same changes as proposed in #3, where there are other breaking changes that aren't just security, performance or bug fixes.

feross commented 8 years ago

Even so, I don't think it's a good idea. Having a deep hierarchy of files should be painful. This makes it easier to make gargantuan modules.

mariusGundersen commented 8 years ago

I agree that it makes it easier to make big modules, but it's up to the developer to decide how they want to structure the application. The places I really miss this feature is in the end applications, those that are not published on npm but end up being deployed to production servers.

kgryte commented 8 years ago

This proposal is not necessary and can already be achieved by placing application files in a top-level node_modules folder.

jkrems commented 8 years ago

I agree with @feross. The current system encourages clean dependency trees and code locality. Making it convenient to "randomly" reach into different parts of the project doesn't sound like an improvement.

Also, any difference in the module resolution rules between import and require would break one of the transitioning scenarios mentioned in the other thread: compiled-to-CJS code and JavaScript module code shipping in the same package. If the module resolution rules would be different between the two, it could lead to lots of breakage once people try node vX (where vX introduced module support).

fatfisz commented 8 years ago

@jkrems Surely "randomly" reaching into the file system is more dangerous than "randomly" reaching into the project?

jkrems commented 8 years ago

@fatfisz require.resolve and passing down resolved paths is a thing that's sometimes necessary. Requiring global config files from known locations isn't completely unreasonable. Sure, not a thing you'd do all the time. But I don't quite understand where you read "because complex dependency graphs are bad, loading dependencies from absolute paths has to be the greatest thing ever".

fatfisz commented 8 years ago

@jkrems I don't know how did you make that deduction at the end, so I'll just ignore that part.

I just see this proposal as an improvement over the current situation. I didn't have any need for requiring files globally, but there were a lot of times when I needed to reference, say, a "config" part of my project from some other part of that project. I think that quite a bit of other people also have this problem given the number of proposed "solutions" in this gist.

Requiring global config files from known locations isn't completely unreasonable. Sure, not a thing you'd do all the time.

I don't see how this isn't applicable to paths relative to the project root.

mariusGundersen commented 8 years ago

Also, any difference in the module resolution rules between import and require would break one of the transitioning scenarios mentioned in the other thread: compiled-to-CJS code and JavaScript module code shipping in the same package. If the module resolution rules would be different between the two, it could lead to lots of breakage once people try node vX (where vX introduced module support).

@jkrems I don't see how adding a feature to import would break things when transitioning from require. How would it break when node vX is released with a new feature that wasn't in node v(X-1)?

There are already plugins that let you compile the syntax @calebmer proposed to CJS: https://github.com/michaelzoidl/babel-root-import

bmeck commented 8 years ago

@calebmer import will not do searching based upon environment variables.

bmeck commented 8 years ago

Are there any major problems with placing links (symlink generally, junction otherwise) in node_modules/ rather than extending the module resolution algorithm?

Given the directory structure:

something/
\-  foo.js
node_modules/
\- something -> ../something

You can use:

require('something/foo.js');
mariusGundersen commented 8 years ago

You can't commit a symlink to git.

Sure, there are ways to get it to work, but they are convoluted and all have some gotchas you have to be careful with. The fact that many people are asking this question (see the gist @fatfisz linked to) indicates that a simple solution, with no external dependencies, is wanted.

fatfisz commented 8 years ago

Also symlinks are not that pleasant to work with on Windows (need elevated permissions even on Admin account).

bmeck commented 8 years ago

@mariusGundersen @fatfisz to my knowledge only Windows git has issues with symlinks. Also use junctions for Windows not symlinks (if you can't use symlinks [due to perms]). See how npm link works. I would suggest a new npm dependency type rather than put this directly in the module resolution spec. Something like npm link --local models lib/models. Mostly just because Windows does not like symlinks and junctions are absolute path only / not across network drives.

fatfisz commented 8 years ago

Using junctions means that the project stops working after moving it elsewhere (e.g. I archive some of my projects by putting them in a separate "archive" directory). Also junctions won't work with git without post-checkout scripts. The same applies to npm link, it needs scripts to work.

All of those solutions can be found in the gist I linked to earlier along with their shortcomings, so the problem is not with which workaround to use - they all have been known for years. This new simple solution would beat them all, because it's platform-independent and doesn't rely on additional config/tools (npm included).

kgryte commented 8 years ago

The simple solution already exists, it is platform-independent, and it doesn't rely on additional config/tools. That solution is to use node_modules. If you are opposed to placing "local" app files in the top-level node_modules directory, then do app/node_modules/foo. And if you want shorter paths or to isolate modules to particular parts of your application, do app/node_modules/foo/node_modules/bar/node_modules/beep/...etc.

Re: config. This is a non-issue. An app should have a centralized configuration. "Reaching" into configs placed in other parts of a project is a bad idea, brittle, and poor design. And with centralized configuration, use IoC, which can be achieved either using singletons or, once again, using node_modules.

Re: tools, such as coverage tools, etc. If files are placed in a local node_modules directory, some care should be taken to ensure that whatever tooling you use does not ignore all node_modules directories by default (e.g., istanbul), instead of just the top-level directory. Aside: tools which do this have poor defaults and should be overridden anyway.

zenparsing commented 8 years ago

The simple solution already exists, it is platform-independent, and it doesn't rely on additional config/tools. That solution is to use node_modules. If you are opposed to placing "local" app files in the top-level node_modules directory, then do app/node_modules/foo. And if you want shorter paths or to isolate modules to particular parts of your application, do app/node_modules/foo/node_modules/bar/node_modules/beep/...etc

Uh, simple?

kgryte commented 8 years ago

@zenparsing Yes. There is nothing complex about it. Node's require algorithm works, satisfies all requirements, and provides flexibility for scoping "modules" to particular parts of an application, not just the root.

fatfisz commented 8 years ago

node_modules is usually used for external deps - it's a de facto standard. And external deps are usually ignored by most tools like linters, test runners, version control, etc., which seems quite natural to me. So putting internal parts of an app with the external ones requires adding exceptions in all those tools' config. I've been through that, and it doesn't seem "simple" at all. Maybe not "complex", but requires additional work, more boilerplate, etc.

kgryte commented 8 years ago

@fatfisz a de facto standard is not a standard. The prohibition against using a node_modules folder anywhere is a common misconception and reflects people's general misunderstanding as to Node's require algorithm and how it should be leveraged within applications (this despite Substack's and Issac's posts to correct this misunderstanding). As mentioned previously, if you are uncomfortable placing application-specific modules in the top-level node_modules directory, place them under a namespaced folder. And no, it does not require more boilerplate. I have used this approach across numerous projects and changes are minimal, the code cleaner, and refactoring easier.

fatfisz commented 8 years ago

The prohibition against using a node_modules folder anywhere is a common misconception and reflects people's general misunderstanding as to Node's require algorithm and how it should be leveraged within applications (this despite Substack's and Issac's posts to correct this misunderstanding).

... reminds me of these situations: image

Just for the record: I've been putting a few of my more complicated projects into node_modules/app for over half a year, but it still looks awful after all this time. All of these complications with node_modules and all these other solutions look pale in comparison with simplicity of putting "/" in front of a path.

And so I have nothing more to add, you can have a productive discussion at last :smile: Peace :heart:

cesarandreu commented 8 years ago

My team has a postinstall script that calls link-package to symlink a few folders to node_modules.

And each folder that gets symlinked gets a package.json, e.g.

{
  "name": "components",
  "standard": {
    "parser": "babel-eslint"
  }
}

At least for frontend apps, I'd say that once it grows to a certain size it becomes a must. It's especially useful for components that get used in all parts of your app.

In your views, you use import { SolidButton } from 'components/Button', instead of ../../components/Button.

This broke slightly with node v6.0.0, due to the change with symlinked folders. Now you also have to update tools like babel to whitelist the symlinks in node_modules.

I'd argue this approach makes it much easier to write modular application code. You have lots of application-specific modules that you can reference without worrying about the location :D. That makes code reuse a breeze.

It's not that big of a deal when you're first implementing something, but as the application evolves due to changing requirements, it makes it much easier to refactor code.

mariusGundersen commented 8 years ago

@kgryte, based on your (and substack's, issac's) explanation of node_modules, why have any code outside of node_modules at all?

This proposal would let the root folder of a project behave almost as a node_modules folder.

bmeck commented 8 years ago

If this is still desired, I would suggest making a new PR, with a different proposal. Going to close this. modules.root from https://github.com/dherman/defense-of-dot-js or https://github.com/bmeck/UnambiguousJavaScriptGrammar would be a good place to start.