Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is this NPM package required when NodeJS ships with a path module? #6

Open
capaj opened this issue Dec 4, 2015 · 15 comments
Open

Is this NPM package required when NodeJS ships with a path module? #6

capaj opened this issue Dec 4, 2015 · 15 comments

Comments

@capaj
Copy link

capaj commented Dec 4, 2015

Why would anyone need to install this module when it comes bundled with npm?

@jinder
Copy link
Owner

jinder commented Dec 4, 2015

Presumably to use it in a browser....

This is a package I've inherited, and it's not actively maintained. I'm really not sure why so many Node-specific packages rely on it. Perhaps it should be deprecated.

@capaj
Copy link
Author

capaj commented Dec 4, 2015

@jinder in the browser? I can't imagine this code would work in browser. process is undefined in the browser, so https://github.com/jinder/path/blob/master/path.js#L25 would throw an error.

For example browserify has it's own path shim which has nor process references:
https://github.com/substack/path-browserify/blob/master/index.js

I think people are installing this, because they don't know this is built in. A lot of people first come to node via something like express tutorial and in it, they have to install everything.

I think this should really be removed from NPM. No need to vaste megabytes of bandwith downloading this.

@jinder
Copy link
Owner

jinder commented Dec 4, 2015

Although it's not packaged (browserify-style), it does depend on the process and util NPM packages. So there's certainly going to be dependencies that are using it and packaging it themselves.

I agree with what you're saying though. But I won't remove it from NPM (hundreds of dependent packages and half a million downloads a month). Perhaps a deprecate and notice would be sufficient?

@capaj
Copy link
Author

capaj commented Dec 4, 2015

@jinder you can't really use this module unless you specify a require by absolute or relative path. So if there is a project, which has this as dependency and requires a path by doing: require('path') it get's the bundled native module not this one. It only sits there and does nothing. I seriously doubt anyone would be requiring path module like this:
require('./node_modules/path')
And if someone does, well, then he had this coming.

I hope NPM maintainers take the same stance.

@jinder
Copy link
Owner

jinder commented Dec 4, 2015

I'm a bit apprehensive about pulling this from the registry without knowing for sure it won't break anything.

I'll deprecate it this weekend and monitor it for a while to see what to do next.

A bit of background about how I've inherited this from @coolaj86: I needed to use a package that depended on this, however it was published to the NPM registry without any licensing information. Which meant the only way I could use it, was by re-publishing it (copy + paste from node core) and updating the package.json with relevant licensing details.

@capaj
Copy link
Author

capaj commented Dec 4, 2015

it should be easy to try out-just find some packages which have CI setup this in package.json and remove it and make a PR. I bet you nothing breaks.

@jinder
Copy link
Owner

jinder commented Dec 4, 2015

You certainly can use it without having an absolute/relative path. RequireJS lets you specify the paths in its config for named modules.

You're welcome to contact any of the dependent libraries to investigate more. For the moment, I think it's only safe to deprecate it.

@jinder jinder changed the title what is the meaning of this? Is this NPM package required when NodeJS ships with a path module? Dec 4, 2015
@coolaj86
Copy link

coolaj86 commented Dec 4, 2015

You could of course add a little love to it like this:

if (process.versions && process.versions.node) {
    console.warn("[path installed via npm]: YOU'RE DOING IT WRONG! See https://github.com/jinder/path/issues/6");
}

@coolaj86
Copy link

coolaj86 commented Dec 4, 2015

Actually... you could do what I do with unibabel's package.json

I set "main": "node.js" and "browser": "unibabel.js".

You can then have the node.js output "you're using this module incorrectly..."

Also, a person can't really accidentally use this from npm. require('path') with always return the internal node module. require('path/'), however, would search node_modules.

@jinder
Copy link
Owner

jinder commented Dec 6, 2015

I think in those instances, you'd only see the message if they're specifically forcing the use of the package under Node. Really, what we want is to somehow notify users who have it listed as a dependency in their package.json, but are actually using the native node module. i.e. it's an unnecessary dependency.

@coolaj86
Copy link

coolaj86 commented Dec 7, 2015

attach a message using npm deprecate?

npm deprecate path@"< 99" "This is for the BROWSER. Do NOT use it for NODE. See https://github.com/jinder/path/issues/6"

@astoilkov
Copy link

You could use the module in a browser environment together with browserify. I have tested this and it works and I am going to use this module for these purposes.

My app runs in node.js and browser environments so this is the perfect module for me.

@jinder
Copy link
Owner

jinder commented Dec 8, 2015

Okay, so, latest plan of action: Update the readme to clearly indicate it's not necessary to have a dependency on this module if you're only running on Node. Any objections? :)

@astoilkov
Copy link

+1 for this

@jinder
Copy link
Owner

jinder commented Dec 15, 2015

Just noticed on NPM 3 at least, the following appears when running install:

npm WARN package.json path@0.12.7 path is also the name of a node core module.

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

No branches or pull requests

4 participants