Remove all relative requires

[r1b]

Consider mandating use of npm link & remove all relative requires. Prevents issues like #2325 as a result of nondeterminism in path.relative(). Also much easier to reason about :)

Maybe not in scope for v4 but definitely for future.

[ericelliott]

What does npm link have to do with relative requires?

[r1b]

npm link gives you a global symbolic link to the folder in which it is run. The name of the link is the same as the "name" field in package.json. You can reference that link in your requires, e.g:

cd ~/path/to/keystone/ && npm link
...
var someLib = require("keystone/path/to/someLib");

[fox1t]

Looking forward this, since keystone broke on 5.7.x

[ericelliott]

This isn't what npm link is for. It should not be required for all apps that use Keystone to link keystone globally. What if you want to run two different apps that use two different versions of Keystone?

[r1b]

I think my initial comment was misleading. I did not mean to mandate the use of npm link for the app that depends on keystone, I meant it should be required to run npm link for development and publishing.

The behavior of npm link that keystone should exploit is (from docs):

When creating tarballs for npm publish, the linked packages are "snapshotted" to their current state by resolving the symbolic links.

In this way, when an application installs keystone via npm, the absolute paths in keystone's requires will be resolved without any extra step for the user. If a user wants to run two different apps that use two different versions of Keystone, they simply have to specify a different version in each package.json (as they normally would).

[geloescht]

To cite npm help link:

First, npm link in a package folder will create a globally-installed
symbolic link from prefix/package-name to the current folder (see npm
help 7 npm-config for the value of prefix)

I highly doubt that this is what you want to do.

Other than that, I support removal of all parent directory requires within keystone. Instead, keystone development and testing should be done in a subdirectory "keystone" of a directory named "node_modules" (as it is always the case, when keystone is used within a server application). Then, requires within keystone can have the form require('keystone/some/component'), and especially, you get rid of ugly and unmaintainable require('../../../') by replacing them with require('keystone').

[ericelliott]

As @geloescht mentioned, npm link creates a global link on the developer's box, so if you're building multiple Keystone apps on one dev box, you'd need them all to be using the same version of Keystone, or you'd need to re-link Keystone every time you switch to a differently versioned Keystone app, which could become an annoying time suck -- especially if you forget and things just seem to be broken for no apparent reason.

[r1b]

@geloescht Creating a global symbolic link is, in fact, what you want to do during development. This is the mechanism that allows you to reference the package by name. When you publish the package, the links are resolved. I don't think that mandating development within a node_modules/ folder is an appropriate solution, you are simply taking advantage of the fact that npm blindly resolves everything inside of that folder.

@ericelliott I believe we have two different definitions of "developer" in this discussion. We have a "keystonejs developer" who works on core keystone features. We also have a "client developer" who uses keystone as a dependency in their project.

In the case of the "keystonejs developer", it is not an issue to work with multiple versions of keystone on one machine. Since keystone is in version control, you just need to switch to a different branch. The symbolic link created by npm link does not need to be recreated. It will resolve to the new version on the same path.

In the case of the "client developer", it is also not an issue to work with multiple versions of keystone on one machine. The client developer will specify the version of keystone that they want in each app's package.json file. The client developer does not need to run npm link.

To be clear, I am only talking about mandating npm link for the "keystonejs developer". FWIW, this is already the recommended way to work on keystone.

[creynders]

I agree with @erg0dic Though npm link is hacky - no discussion there - it's by design, however the node_modules approach will open up a can of worms, where more and more stuff will be moved into the node_modules directory. It's an ok approach for some situations - like extracting stuff that'll become a separate module somewhere down the road - but not in this case.

[creynders]

Obviously this needs to be tested thoroughly from both developer's view points to make sure we're not missing something important here.

[creynders]

And come to think of it, maybe this warrants a feature request for node (or npm? I never know what require stuff goes where): when a file require's subparts of its own package it should resolve to that package's subparts. Ok, that's not very clear, but I think you know what I mean.

[geloescht]

@erg0dic : To cite the paragraph you linked:

If you want to test or develop against the master branch of KeystoneJS [...]

This is about developing a app using a development branch.

Anyway, it's dangerous. Imagine keystone developer Maurice has two versions of keystone checked out at the same time. One in /home/maurice/development/keystone-master/ and one in /home/maurice/development/keystone-maurices-new-feature/.
He does npm link in keystone-maurices-new-feature in order to test the new feature in an app at /home/maurice/development/maurices-keystone-app. Unfortunately, some things broke while developing the new feature.
Then he fixes some serious bugs in keystone-master and would like to test before pushing the repository. But no!! npm test fails! Because it required the globally linked version of keystone in keystone-maurices-new-feature. And that is because node's resolution algorithm looks at the global folder when nothing has been found locally.

The problem exists, because require has one serious gap: It does not consider the current package it is in. Unless: You develop inside a directory named node_modules. Or you link to '..' inside the sub-directory node_modules. npm link does the second thing, but poorly, by going through a global, machine-dependent location.

If there is a way of invocation npm link, that does not link to a global location, fine. Otherwise, better stay away from it and use good old ln -s, when you want to have links.

[creynders]

Yeah, that's the one use case which is potentially dangerous. But TBH, once you start messing with npm link for whatever reason - i.e. Maurice would be npm linking anyway, regardless of this discussion - you have to make sure that whatever you're linking to is the correct thing. A long-winded way to say: Maurice would be in trouble in the current situation anyway.

[geloescht]

Well, if by current situation you are referring to the one with relative requires: No trouble at all.
Also not if Maurice didn't rely on npm link to resolve require('keystone'), by a) developing within a directory named "node_modules/keystone" or b) creating a symbolic link to '..' named "keystone" within keystone/node_modules.
Especially a) should be extremely safe as long as you keep this directory node_modules empty except for keystone. On the other hand, b) has the advantage of being self-contained within a git repository, however I have no experience on how npm behaves in the presence of recursive links.

[r1b]

@geloescht You are correct. In the situation you described, you would have to run npm link again in "keystone-master" to point to the correct version of keystone. However, the only thing that makes this situation dangerous is the fact that you have multiple versions of keystone checked out across multiple directories.

I don't mean to try to tell you how to work, but it would be much more efficient to leverage git to manage multiple versions of keystone in a single directory. I believe that this is how most people work with git. If you manage multiple versions of keystone in this way, npm link will always resolve to the revision that is currently checked out. If you would like to make a change on another revision, you simply checkout that revision.

[geloescht]

Having multiple versions of a repository is quite handy if you do not want to stash all your uncommited changes in a feature branch before working on a different branch.
The thing that makes npm link dangerous is, that it puts stuff into a location that is specific to your machine and this location usually happens to be in your NODE_PATH. It is like npm install -g. The fact that it resolves internal requires when doing npm run test is purely coincidental.

IMO, npm link is a hack and has no advantage over ln -s except that it is a little more convenient in usage. The good thing for both of us is: The behaviour of npm publish relating to symbolic links is identical in both cases. Most this discussion is academic and more about personal preference. Just, please don't make npm link mandatory for keystone development.

[creynders]

@geloescht Probably you have a different workflow, but I meant that in the situation you describe:

Anyway, it's dangerous. Imagine keystone developer Maurice has two versions of keystone checked out at the same time. One in /home/maurice/development/keystone-master/ and one in /home/maurice/development/keystone-maurices-new-feature/.

Most people will be npm link juggling anyway, regardless of what we're discussing here. At least I do, since for both /home/maurice/development/keystone-master/ and /home/maurice/development/keystone-maurices-new-feature/ I'd test them out in a test project npm linked to keystone. I.e. this discussion doesn't really matter to the situation you describe, IMO.

What you propose is far more invasive and - again IMO - equally hacky, which is why I definitely prefer @erg0dic 's solution.

[geloescht]

I do link juggling using ln -s and not npm link, because ln -s does not modify my global environment.
npm link does not help with npm run test, which is the only occasion that I can think of, when you want to run keystone code outside a folder named node_modules. That means, it is the only occasion when require('keystone') does not refer to the current package, but potentially to anything that happens to be inside your NODE_PATH.

[r1b]

There are a number of ways to deal with relative requires in development. Here is a good list. I suggested using npm link because:

• This is how we deal with the problem at my workplace & it has served us well
• Keystone already suggests using npm link for local development, so it should be painless for existing keystone developers

I am not here to prove the relative superiority of my method over any other. I would, however, like us to agree to do something to address the issue of relative requires.

[creynders]

@JedWatson any thoughts on this? It sure would be nice to get rid of the relative requires

[geloescht]

By the way, I submitted a pull request #2390 that depends on switching to module paths instead of relative paths. So I would love to hear a decision on this matter 🐵
(There I have used the symlink strategy I described above to fix Travis CI for now and have not seen any adverse effects until now. But it is known to break on Windows, so a more clever solution than just including a symlink in the repo must probably be found)

[JedWatson]

Works on Windows is unfortunately a non-negotiable requirement for Keystone development, as at least one of our core contributors uses it and I don't want to cause problems for him (hi @webteckie 👋) or alienate other potential contributors on Windows.

I don't see any issues using ln -s for local development if that works for you (rather than npm link) but while linking is the officially blessed solution in node, I think it should be the officially blessed solution for Keystone development. I often work with a complex spider web of npm linked packages (including elemental and keystone-utils as examples) and while it takes management, it does work universally and is (as far as I can see) the "expected" behaviour for modules.

I'm all for resolving the relative requires if we can, but not as the cost of introducing hacks into the core codebase, especially if they have stability issues across platforms or may run into breaking changes with newer versions of node. They're a PITA when coding, but stability and predictability need to be our priorities as Keystone matures.

[webteckie]

Alright then I admit @JedWatson :-) fyi, while on windows I do spend 100% of the time in git bash and use all linux commands :-) Not sure if symbolic links work there...will check. But it's just one of those cases where I haven't had the need to buy a mac! But I've been thinking hard about buying one :-)

[mxstbr]

As long as it works, do you really need to switch? 👍

I agree with Jed, we should definitely keep this as OS friendly as humanly possible.

[webteckie]

@mxstbr gonna start doing some iOS mobile app dev. But I realized I can also rent a mac in the cloud :-)

[geloescht]

Alright then, I give in :)
So on the technical side, I think some scripting needs to be done in order for Travis CI to keep working (I have never used it before, so I chose the easy path in my PR). Humans on the other hand should be instructed via the docs, I guess.

Whereas the title says, remove all relative requires, I think it should only be most relative requires. I believe where tight coupling is already in place, such as where methods of a prototype (don't call it class ;) ) are divided up into separate files, relative queries between those files can stay (lib/list comes to mind). If they get moved around or referred to from the outside, then probably as one big chunk.

Actually, this whole discussion has got me thinking of developing a non-evil version of npm link 😎

[creynders]

Actually, this whole discussion has got me thinking of developing a non-evil version of npm link

💯 that would be great

[offero]

💯 that would be great

Seconded.

[mxstbr]

I'll close this issue for now, since #2390 is not directly related to this!