search

browserify / resolve

Implements the node.js require.resolve() algorithm
MIT License
776 stars 184 forks source link

All files are included into npm #238

Closed JounQin closed 3 years ago

ljharb commented 3 years ago

This is quite intentional. Those files must be included.

Tests should be published with every package, so that npm explore foo && npm install && npm test always works.

Installed package size is irrelevant; if you never require the test files, they'll never affect the runtime of your application.

Duplicate of #235. Duplicate of #58. Duplicate of #44. See https://github.com/browserify/resolve/pull/105#issuecomment-266653976.

JounQin commented 3 years ago

@ljharb

I didn't get the point why npm test should be run inside node_modules.

If you want to test, you can always use git version to install the package.

I raised the PR because when I open the file in VSCode, vscode-eslint reports @ljharb config not found because there is a .eslintrc in the package.

ljharb commented 3 years ago

why would you be trying to lint files in node_modules (when not cding into the package directly)? that seems like vscode-eslint is broken.

I can't, in fact, always use the git version, because I might not have internet access when I need to run the tests.

Either way, whether you get the point or not is irrelevant; it's a critical use case for me, and every package I maintain will always publish all needed files to npm, and it's a bug if they don't.

JounQin commented 3 years ago

why would you be trying to lint files in node_modules (when not cding into the package directly)? that seems like vscode-eslint is broken.

It's automatically done by vscode-eslint. You can say vscode-eslint is broken, or not. Because there is a .eslintrc, why not read it?

I can't, in fact, always use the git version, because I might not have internet access when I need to run the tests.

But for this package, I suppose you always have the access, didn't you?

Either way, whether you get the point or not is irrelevant; it's a critical use case for me, and every package I maintain will always publish all needed files to npm, and it's a bug if they don't.

Well, again, you're the boss here. I just want say that this package is very popular, and the installation size is also important. I understand your own workflow, that's great. But as you said there are duplicate attempts to resolve this problem, then it's indeed a problem for others, I hope you can consider it more carefully.

ljharb commented 3 years ago

I don't find installation size to be that important - it's all tarballed, and untarring it, whether large or small, is basically equally instantaneous.