Fix: Deprecate implicit file: protocol and only allow package dirs (#…

…4257)

**Summary**

Fixes #4251. Follow up to #4088. Instead of just checking whether the
target is a valid directory, we now check if it contains a
`package.json` file too. This is still different from `npm`'s behavior.
Apparently, `npm` fetches the package info upfront to favor dist-tags
over directories but this comes at a distinct performance penalty and
makes static, deterministic resolution impossible so we are now
deprecating the implicit `file:` protocol in patterns. After a certain
point, we'll remove this code and will require everyone to use `file:`
or at least one of the following path identifiers: `./`, `../`. `/`.

**Test plan**

Updated the existing test for warning check and added a new test for
invalid directories.

__tests__/commands/install/integration.js

@@ -442,13 +442,13 @@ test.concurrent('install file: local packages with local dependencies', async ()
 });
 
 test.concurrent('install file: install without manifest of dependency', async (): Promise<void> => {
-  await runInstall({}, 'install-file-without-manifest', async (config, reporter) => {
+  await runInstall({}, 'install-file-without-manifest', async config => {
     expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('bar\n');
   });
 });
 
 test.concurrent('install file: link file dependencies', async (): Promise<void> => {
-  await runInstall({}, 'install-file-link-dependencies', async (config, reporter) => {
+  await runInstall({}, 'install-file-link-dependencies', async config => {
     const statA = await fs.lstat(path.join(config.cwd, 'node_modules', 'a'));
     expect(statA.isSymbolicLink()).toEqual(true);
 
@@ -471,8 +471,10 @@ test.concurrent('install file: protocol', (): Promise<void> => {
 });
 
 test.concurrent('install with file: protocol as default', (): Promise<void> => {
-  return runInstall({}, 'install-file-as-default', async config => {
+  return runInstall({}, 'install-file-as-default', async (config, reporter, install, getOutput) => {
     expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'index.js'))).toEqual('foobar;\n');
+
+    expect(getOutput()).toContain(reporter.lang('implicitFileDeprecated', 'bar'));
   });
 });
 
@@ -481,6 +483,13 @@ test.concurrent("don't install with file: protocol as default if target is a fil
   return expect(runInstall({lockfile: false}, 'install-file-as-default-no-file')).rejects.toBeDefined();
 });
 
+test.concurrent("don't install with file: protocol as default if target does not have package.json", (): Promise<
+  void,
+> => {
+  // $FlowFixMe
+  return expect(runInstall({lockfile: false}, 'install-file-as-default-no-package')).rejects.toBeDefined();
+});
+
 test.concurrent("don't install with file: protocol as default if target is valid semver", (): Promise<void> => {
   return runInstall({}, 'install-file-as-default-no-semver', async config => {
     expect(await fs.readFile(path.join(config.cwd, 'node_modules', 'foo', 'package.json'))).toMatchSnapshot(

__tests__/fixtures/install/install-file-as-default-no-package/package.json

@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "foo": "bar"
+  }
+}

__tests__/integration.js

@@ -84,7 +84,6 @@ test('--mutex network', async () => {
 
 test('--cwd option', async () => {
   const cwd = await makeTemp();
-  const cacheFolder = path.join(cwd, '.cache');
 
   const subdir = path.join(cwd, 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i');
   await fs.mkdirp(subdir);

src/package-request.js

@@ -5,6 +5,12 @@ import type PackageResolver from './package-resolver.js';
 import type {Reporter} from './reporters/index.js';
 import type Config from './config.js';
 import type {Install} from './cli/commands/install';
+
+import path from 'path';
+
+import invariant from 'invariant';
+import semver from 'semver';
+
 import {cleanDependencies} from './util/normalize-manifest/validate.js';
 import Lockfile from './lockfile';
 import PackageReference from './package-reference.js';
@@ -17,10 +23,6 @@ import {getExoticResolver} from './resolvers/index.js';
 import * as fs from './util/fs.js';
 import {normalizePattern} from './util/normalize-pattern.js';
 
-const path = require('path');
-const invariant = require('invariant');
-const semver = require('semver');
-
 type ResolverRegistryNames = $Keys<typeof registryResolvers>;
 
 export default class PackageRequest {
@@ -127,7 +129,8 @@ export default class PackageRequest {
 
     if (!semver.validRange(pattern)) {
       try {
-        if ((await fs.stat(path.join(this.config.cwd, pattern))).isDirectory()) {
+        if (await fs.exists(path.join(this.config.cwd, pattern, constants.NODE_PACKAGE_JSON))) {
+          this.reporter.warn(this.reporter.lang('implicitFileDeprecated', pattern));
           return `file:${pattern}`;
         }
       } catch (err) {

src/reporters/lang/en.js

@@ -348,6 +348,8 @@ const messages = {
   scopeNotValid: 'The specified scope is not valid.',
 
   deprecatedCommand: '$0 is deprecated. Please use $1.',
+  implicitFileDeprecated:
+    'Using the "file:" protocol implicitly is deprecated. Please either the protocol or prepend the path $0 with "./".',
 };
 
 export type LanguageKeys = $Keys<typeof messages>;

src/resolvers/exotics/file-resolver.js

@@ -1,5 +1,10 @@
 /* @flow */
 
+import path from 'path';
+
+import invariant from 'invariant';
+import uuid from 'uuid';
+
 import type {Manifest} from '../../types.js';
 import type PackageRequest from '../../package-request.js';
 import type {RegistryNames} from '../../registries/index.js';
@@ -8,10 +13,6 @@ import ExoticResolver from './exotic-resolver.js';
 import * as util from '../../util/misc.js';
 import * as fs from '../../util/fs.js';
 
-const invariant = require('invariant');
-const path = require('path');
-const uuid = require('uuid');
-
 export const FILE_PROTOCOL_PREFIX = 'file:';
 
 export default class FileResolver extends ExoticResolver {
@@ -23,6 +24,11 @@ export default class FileResolver extends ExoticResolver {
   loc: string;
 
   static protocol = 'file';
+  static prefixMatcher = /^.{1,2}\//;
+
+  static isVersion(pattern: string): boolean {
+    return super.isVersion.call(this, pattern) || this.prefixMatcher.test(pattern) || path.isAbsolute(pattern);
+  }
 
   async resolve(): Promise<Manifest> {
     let loc = this.loc;