Use nerf gun

[indexzero]

@othiym23 @isaacs

Supercedes #6514. Corresponding fix for npm/npm-registry-client#77. Moved the duplicated toNerfDart implementation across npm/npm and npm/npm-registry-client to nerf-gun and applied the appropriate fix.

[terinjokes]

The code changes look good to me.

Seems strange that you're adding nerfGun to bundledDependencies, but the PR doesn't include the package in node_modules. The Contributing Guidelines unfortunately don't have much to say on this issue. /cc @othiym23 to hopefully clear up what a PR should or should not have.

[indexzero]

Thanks for the tip @terinjokes. I added it to bundleDependencies to pass 00-verify-bundle-deps.js, but did not check it in to node_modules.

There does seem to be some standard around this from @othiym23. See this recent commit updating to sha@1.3.0

Happy to do whatever is the norm here.

[iarna]

Yes, that's right, all modules required by npm need to be checked in and in bundleDependencies. (devDependencies obviously don't count for this.)

[indexzero]

@iarna thanks, is there anything special about the dependencies themselves? e.g. should I not check-in files that are matched by a .npmignore file?

[iarna]

@indexzero Nothing special– I mean, just what you get after npm install. Obviously, if you've got editor or testing artifacts in there those shouldn't be included.

[indexzero]

@iarna thanks. Published nerf-gun@1.0.1 with a proper .npmignore which ignores the test file. Then packaged that all up into a single commit with the addition to node_modules.

Look OK now?

[jcrugzz]

@aredridel i believe that was per @iarna's suggestion in order to ensure minimal additions since all the deps are bundled here.

[iarna]

@jcrugzz That was not my suggestion (look at the other packages in npm, we always include tests)

[jcrugzz]

@iarna gotcha, you mentioned testing artifacts. Seems that the base example of sha did not include the test directory.

[indexzero]

@iarna yes, @jcrugzz is correct. I was following the example of sha

[shellscape]

👍 has my vote

[cblage]

👍

[ecwall]

+1

[cronopio]

👍 +1

[indexzero]

@iarna what's the verdict then? Should I include the test/ directory or not since there is some inconsistency across modules taken as dependencies by npm? I wrote a quick module which told me exactly which modules npm depends on ignores tests:

npm install -g npmignore-tests
cd /path/to/npm
npmignore-tests .

which yields:

glob ignores [test/a/]
marked ignores [test/]
marked-man ignores [test/files/*.roff, test/files/*.err]
nerf-gun ignores [test/*]
node-gyp ignores [gyp/test]
npm-registry-client ignores [test/fixtures/cache]
npm-registry-couchapp ignores [test/fixtures/npmrc*]
readable-stream ignores [test/]
request ignores [tests]
sha ignores [test]
tar ignores [test/tmp/, test/fixtures/]

So it's not especially common, but some notable modules like marked, request, readable-stream, and sha do this. I'm happy to re-add them if that helps get this accepted, just trying to be consistent.

[othiym23]

This has been supplanted by npm-registry-client@4.0.0, which is included in npm@2.1.7. See also 20a331c, which incorporates your tests.