Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

local path support in package.json #5629

Closed
wants to merge 14 commits into from

Conversation

dylang
Copy link
Contributor

@dylang dylang commented Jul 4, 2014

Adds support for package.json to have local paths, such as relative paths to packages which is helpful for testing. Also add support for --save, --save-dev to save local paths to package.json.

This replaces #5570.

As recommended:

  • The prepublish validation has been removed.
  • Tests added. All tests should pass when couchdb is installed.
  • Short circuit checks file path before assuming github shortcut for both command line and package.json.

fixes #2442
fixes #4018

and possibly others.

@PaulHuizer
Copy link

This is really nice. +1

@othiym23
Copy link
Contributor

This looks nice. Update the documentation to explain the new functionality and I'll merge it. Thanks for your patience, Dylan!

@dylang
Copy link
Contributor Author

dylang commented Jul 21, 2014

Awesome, thanks @othiym23!

Docs added, though let me know if you want more.

@timoxley
Copy link
Contributor

allow local paths in package.json, but disallow publishing

Could this be limited to only paths pointing outside the package's hierarchy?

@hughsk and I have been building/using a tool which allows internal dependencies via symlinking into node_modules, allowing us to depend on a local ./lib/mything and have mything have its own package.json, bin scripts, tests, etc. This allows for far more modular construction, as it reduces most of the overhead of creating a modular application i.e. doesn't require having to do the "update deps, version bump, publish" dance across multiple internal structures which make no sense existing outside of the current codebase.

i.e. how a package chooses to organise its child dependencies should not affect the parent package's ability to be published, so long as it's not reaching outside of its own hierarchy.

@dylang
Copy link
Contributor Author

dylang commented Jul 21, 2014

allow local paths in package.json, but disallow publishing

@timoxley - great point, and partly for this reason this PR does not include the "disallow publishing when local paths are used" code.

The part of my notes that says "The prepublish validation has been removed." were intended to mean that, sorry for the confusing wording.

@timoxley
Copy link
Contributor

@dylang I see that now. Should stuff be able to be published if it depends on ../../something ?

@dylang
Copy link
Contributor Author

dylang commented Jul 21, 2014

@timoxley I don't think it should either. @othiym23 requested (last paragraph) I take that part of the PR out for a separate review. My previous code didn't allow any local directories, but I agree directories within the package should be valid.

@dylang
Copy link
Contributor Author

dylang commented Jul 23, 2014

Pulled in the latest from master.

@othiym23 - I'd love to see this in a 2.0.0-alpha build so devs might provided feedback.
What else can I do to help?

@othiym23
Copy link
Contributor

It's on the list! Just need a chance to do a final review / tweak / merge, probably in the next day or so.

@dylang
Copy link
Contributor Author

dylang commented Jul 23, 2014

excited

@mice530
Copy link

mice530 commented Jul 24, 2014

@dylang when coming across the private package that is served on http service with authorization needed, there is no way for now to input username & password

@dylang
Copy link
Contributor Author

dylang commented Jul 24, 2014

Hi @mice530, thanks for the feedback. Is this a new issue caused by this PR? Can you provide an example of what used to work but doesn't anymore? For security reasons, don't include real passwords in your example.

Alos, is #4853 related?

@othiym23
Copy link
Contributor

@mice530, I think the issue you're encountering is tied to #5724 / #4853, and is unrelated to @dylang's change. It's in the process of being addressed right now.

, isLocal

try {
isLocal = fs.statSync(t.from);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really try to keep sync code out of the core of npm, because it can totally jam up installs if a lot of things are happening at once. Can you rework this to be async?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised when you didn't comment on this glaring sync call earlier. I hate it too.

@othiym23
Copy link
Contributor

@dylang I'm seeing some test failures in the npm-registry-couchapp test. Could you install CouchDB in your development environment and take a look at those failures? It may require another pull request (on npm-registry-couchapp) to fix properly. Sorry! 😑

@othiym23
Copy link
Contributor

Also, if you wanted to add a thing to it so that local dependencies are rewritten to be file: URLs before they're saved to package.json, that would be super awesome. It would go a long way towards making clear to users who run into local dependencies what's going on.

@dylang
Copy link
Contributor Author

dylang commented Jul 25, 2014

some test failures in the npm-registry-couchapp

Yup, I have it installed, and I see the failures. I was hoping they were unrelated to my PR. I'll look into that.

@dylang
Copy link
Contributor Author

dylang commented Jul 25, 2014

local dependencies are rewritten to be file: URLs before they're saved to package.json

This makes sense.

I might be able to remove the ugly isLocal = fs.statSync(t.from); in favor of looking for file: prefix.

Do you think file:-style URLs need to be valid (or required) on the command line/api?

@dylang
Copy link
Contributor Author

dylang commented Jul 25, 2014

@othiym23 How is this:

  • file:///relative/child/path
  • file:///../relative/parent/path

All paths are relative to the project because I'm not sure a good way to support absolute and relative paths without it looking really ugly, such as file:///./relative/path.

It has to be /// because we are leaving out the hostname. We could put localhost there.

http://en.wikipedia.org/wiki/File_URI_scheme
http://tools.ietf.org/html/rfc1738

@dylang
Copy link
Contributor Author

dylang commented Jul 25, 2014

I think i need to update npm-package-arg's parseUrl to recongize file: URLs.

@othiym23
Copy link
Contributor

@dylang here is my preference:

file:relative/child/path.js
file:../relative/parent/path.js
file:///absolute/path.js
file://C:/absolute/path.js (legacy Windows path)
file:///?/c:/absolute/path.js (Windows UNC path)

The RFC wants the two leading slashes, but without is enough in line with existing practice that I'll be able to sleep at night (importantly, require('url').parse will do the right thing with them) and it's way less confusing for relative paths without them there.

I promise you that I'm not trolling you with those Windows paths, but this does raise the question of how absolute Windows paths should be stored in package.json. Let's just say that as long as the file:// prefix is there for absolute paths, it doesn't matter which of the two styles a given path uses.

Yeah, npm-package-arg will need to be updated; you can probably take a look at using url.parse to make that fairly simple. And take a look at #5801, which was directly inspired by this discussion.

@dylang
Copy link
Contributor Author

dylang commented Jul 26, 2014

Yeah, npm-package-arg will need to be updated;

npm/npm-package-arg#3

I promise you that I'm not trolling you with those Windows paths

Since require('url')` doesn't quite get those paths I'd like to hold off with absolute paths. I don't recall seeing requests for absolute paths.

And take a look at #5801, which was directly inspired by this discussion.

Interesting, but I think should be handled in a separate PR.

@othiym23
Copy link
Contributor

Interesting, but I think should be handled in a separate PR.

Absolutely, I was just pointing out that this idea is spreading!

Since require('url')` doesn't quite get those paths I'd like to hold off with absolute paths.

I'll take a look at what it would take to add absolute paths myself, then, because I'm uncomfortable with having a partial solution in there, particularly if it breaks things for Windows but not unix paths.

@dylang
Copy link
Contributor Author

dylang commented Jul 26, 2014

I just realized the one test still not passing isn't passing in master either.

not ok test/tap/peer-deps.js ............................ 1/2
...
"problems" : ["extraneous: request@0.9.5 /Users/dgreene/projects/pr/npm/test/tap/peer-deps/node_modules/request"], // != undefined
...

@mice530
Copy link

mice530 commented Jul 29, 2014

Hi @dylang @othiym23 , #4853 is partly related, but in my case, I need an independent package served in http service which needed username and password, however, all others dependences are public and can be fetched from npmjs.org.

Example:
dependences: {
"connect": "^1.0.01",
"privateMod": "http://someHttpServerNeedUserAndPassword/privateMod"
}

Has this case been addressed?

@othiym23
Copy link
Contributor

@mice530 This sounds like an issue unrelated to @dylang's changes, which are exclusively for adding support to local paths on the same fileserver filesystem, not to http: or https: dependencies. I really think #4853 is the issue that handles your use case, and it will be addressed by a separate fix I'm working on in the near future.

@mice530
Copy link

mice530 commented Jul 29, 2014

All right, I'll move to #4853. Thank you @othiym23 @dylang

@othiym23
Copy link
Contributor

othiym23 commented Sep 5, 2014

It was a bit of a bear to get working with the changes that have since been landed on master, but I landed this as 991484a..4067d6b, and also added

9164acb
16073e2
1acb174
bf70c95
d845e9d

to get everything squared up. I also had to add d2b1c5e to make this work with PR #6119, so be sure to run npm@2.0.0-beta.3 through its paces and make sure this feature is working the way you expect. In particular, when saving file: URLs, the current versions always normalizes it to a relative path, because the old strategy was breaking and this was the easiest and cleanest way to fix it.

Thanks for putting this together, and thanks for your patience!

@othiym23 othiym23 closed this Sep 5, 2014
@dylang
Copy link
Contributor Author

dylang commented Sep 5, 2014

Many thanks @othiym23! I really appreciate all your work to get this in.

@clayallsopp
Copy link

hey folks, not super familiar with npm's implementation, but is there a strong reason not to use symlinks for local paths? (or at least have an option for it, such as symfile:// etc)

our use case is we're usually iterating on modules simultaneously, and it's a bummer to npm install every time we want to get the cross-module code changes to appear

(there are external tools that manage this, such as one linked earlier, but might be nice to have built-in support)

@othiym23
Copy link
Contributor

@clayallsopp You may want to take a look at npm link.

@clayallsopp
Copy link

@othiym23 yup am aware of that, but I think it's suboptimal for our kind of workflow.

I think the ideal API is a declarative way of doing this in package.json:

{
  "dependencies": {
    "lib-core" : "symlink://../lib-core",
    "lib-api"   : "symlink://../lib-api"
  }
}

this seams reasonably close to the file://API, except that it creates symlinks instead of copies

using npm link is currently done inside a preinstall script, but it's a bit cludgy (i.e. iterate through all the folders, recursively figure out where the links need to be made, etc)

(not sure if this is a use case you'd want to address and totally understand if it's not)

@tnrich
Copy link

tnrich commented Sep 11, 2015

@clayallsopp , I think what you're suggesting is a great idea. I'd like to develop modules locally, but dislike having to constantly re-install them. Using npm link via a pre-install is indeed very clunky.

@tnrich
Copy link

tnrich commented Sep 11, 2015

I just found the npm package linklocal which auto-symlinks locally installed npm modules and does just what I'm looking for. Thanks @timoxley for that!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants