Repository: vercel/nft (javascript)

Issue:
Dependencies required through `module-alias` are treated as asset in 0.22.0
## What's wrong

In the recent release of `nft@0.22.0` and Vercel internal builder update(<24hrs), users who deployed [RSSHub](https://github.com/DIYgod/RSSHub) on Vercel no longer works.
The function logs show that

```bash
2022-08-29T20:12:19.574Z	undefined	ERROR	Cannot find module 'got'
Require stack:
- /var/task/lib/utils/got.js
- /var/task/lib/middleware/parameter.js
- /var/task/lib/app.js
- /var/task/api/vercel.js
2022-08-29T20:12:19.574Z	undefined	ERROR	Did you forget to add it to "dependencies" in `package.json`?
RequestId: aecf3542-8033-4686-b5e8-30cdec421dc9 Error: Runtime exited with error: exit status 1
Runtime.ExitError
```

Upon inspections, it's found that `nft@0.22.0` will categorise modules that are required through [`module-alias`](https://www.npmjs.com/package/module-alias) as `asset` and will be dropped. In the above log, `got` is treated as `asset` and removed from `node_modules`.

## Steps reproduce

Repo link to reproduce this issue [TonyRL/RSSHub-nft-repro](https://github.com/TonyRL/RSSHub-nft-repro)

1. Deploy it using <https://vercel.com/import/project?template=https://github.com/TonyRL/RSSHub-nft-repro>
2. Visit \<deployed domain\>/github/comments/vercel/nft/304
3. Expect the above logs

## Somes Observations

Since we also use `nft` as a source to minify our docker image, with a slight modification to the script, here are the `reasons` from `nft@0.21.0` and `nft@0.22.0`.
[reasons-v0.21.log](https://github.com/vercel/nft/files/9447867/reasons-v21.log)
[reasons-v0.22.log](https://github.com/vercel/nft/files/9447869/reasons-v22.log)

### Steps to reproduce these logs

1. `yarn`
2. `node scripts/docker/minify-docker.js > reasons-v0.22.log`
3. roll back to `nft@0.21.0` using `yarn add -D nft@0.21.0`
4. `node scripts/docker/minify-docker.js > reasons-v0.21.log`

### observations(cont.)

For simplicities, you can focus on `lib/utils/got.js`(our wrapper of `got`) only. In `nft@0.21.0`, `lib/utils/got.js` is a dependency and `got` is `resolve`:

```js
  'lib/utils/got.js' => {
    type: [ 'dependency' ],
    ignored: false,
    parents: Set(5) {
      'api/vercel.js',
      'lib/routes/docschina/jsweekly.js',
      'lib/routes/jinritoutiao/keyword.js',
      'lib/routes/shanbay/checkin.js',
      'lib/routes/universities/hit/jwc.js'
    }
  },
  ...
  'node_modules/got/package.json' => {
    type: [ 'resolve' ],
    ignored: false,
    parents: Set(27) {
      'lib/utils/got.js',
      'lib/routes/vscode/marketplace.js',
      'lib/routes/feed-the-beast/modpack.js',
      'lib/routes/gvm/index.js',
      'node_modules/got/dist/source/index.js',
      ...
    }
  },
```

In `nft@0.22.0`, `lib/utils/got.js` becomes an asset and `got` no longer appears in the `reasons` list:

```js
  'lib/utils/got.js' => {
    type: [ 'asset' ],
    ignored: false,
    parents: Set(1) { 'api/vercel.js' }
  },
```

If we require got using relative path instead of going through `module-alias` in `lib/middleware/parameter.js`(from `const got = require('@/utils/got');` to `const got = require('../utils/got');`) then `lib/utils/got.js` becomes one of the dependencies:

```js
  'lib/utils/got.js' => {
    type: [ 'asset', 'dependency' ],
    ignored: false,
    parents: Set(2) { 'api/vercel.js', 'lib/middleware/parameter.js' }
  },
```

and its dependency `got` will also be included in the `reasons`:
[reasons-v0.22-parameter.log](https://github.com/vercel/nft/files/9448083/reasons-v22-parameter.log)

```js
  'node_modules/got/package.json' => {
    type: [ 'resolve' ],
    ignored: false,
    parents: Set(24) {
      'lib/utils/got.js',
      'node_modules/got/dist/source/index.js',
      'node_modules/got/dist/source/create.js',
      ...
    }
  },
```

## Additional info

- `module-alias` works in the application by defining the alias in `lib/app.js`

```js
const moduleAlias = require('module-alias');
moduleAlias.addAlias('@', () => __dirname);
```

- Promoting the working deployment to production will not work as I believe Vercel's internal builder starts using `nft@0.22.0` in roughly <24hrs ago. 


Discussion:
Hi @TonyRL 

The previous version of nft was incorrectly including files as dependencies instead of assets. It was a coincidence that it worked with your repository, likely because the `path.join()` was being globbed and traced when it should have been an asset.

I looked at [module-alias](https://www.npmjs.com/package/module-alias) and it does not seem to be statically analyzable since the callback needs to be evaluated.

However, there is a native feature of Node.js you can use to achieve the same result, [package.json "imports"](https://nodejs.org/api/packages.html#imports), which will work with nft.

It looks something like this:

```json
  "imports": {
    "#got": {
      "default": "./lib/utils/got.js"
    }
  }
```

And you can use it like this: 


```js
const got = require('#got')
```
I realize this can take some time to update your code.

So in the meantime, you can make deployments with a previous version of Vercel CLI (and thus, the previous version of nft) by adding the environment variable `VERCEL_CLI_VERSION=vercel@28.1.4` to your Vercel Project.

I would not recommend that as a long term solution since it will prevent updates, but it could be a short term workaround and give you time to update your code to use package.json imports.
> However, there is a native feature of Node.js you can use to achieve the same result, [package.json "imports"](https://nodejs.org/api/packages.html#imports), which will work with nft.

Thanks for bringing this up. Looks like it's the way to go.

However, after updating the code to use subpath imports, I found out that `nft` does not support 'lazy-requiring' dependencies(with `imports`). This is confirmed by running `node scripts/docker/minify-docker.js > reasons.log` and `nft build api/vercel.js`.
I've also bumped the code in the reproducible repo [TonyRL/RSSHub-nft-repro](https://github.com/TonyRL/RSSHub-nft-repro).

For example, if you visit `<deployed domain>/1draw`, it will require `./routes/1draw/index.js` at runtime by `lazyloadRouteHandler` in `lib/router.js`. Since `nft` can't recognise 'lazy-required' files at build time(not even appears in `reasons`), it throws:
```bash
: Error in /1draw: Cannot find module './routes/1draw/index'
Require stack:
- /var/task/lib/router.js
- /var/task/lib/app.js
- /var/task/api/vercel.js
```

Again, similar behaviour also happens to `<deployed domain>/github/comments/vercel/nft/304`. But in this case, `lib/v2/github/comments.js` treated as `asset` and the functions log shows:
```bash
: Error in /github/comments/vercel/nft/304: Cannot find module 'markdown-it'
Require stack:
- /var/task/lib/v2/github/comments.js
- /var/task/lib/v2/github/router.js
- /var/task/node_modules/require-all/index.js
- /var/task/lib/v2router.js
- /var/task/lib/middleware/load-on-demand.js
- /var/task/lib/app.js
- /var/task/api/vercel.js
```

## Edit

Some interesting findings: Looks like `nft` can handle some kind of 'lazy-loading' if [module-alias](https://www.npmjs.com/package/module-alias) is used.

Below are 2 `reasons` generated with the same script above after I change the code to use `imports`.
[reasons-imports-v0.21.log](https://github.com/vercel/nft/files/9463522/reasons-imports-v0.21.log)
[reasons-imports-v0.22.log](https://github.com/vercel/nft/files/9463523/reasons-imports-v0.22.log)

|  | module-alias | "imports" |
|---|---|---|
| nft@0.21.0 | - lib/routes/1draw/index.js: dependency<br>- lib/v2/github/comments.js: dependency | - lib/routes/1draw/index.js: not in list<br>- lib/v2/github/comments.js: dependency |
| nft@0.22.0 | - lib/routes/1draw/index.js: asset<br>- lib/v2/github/comments.js: asset | - lib/routes/1draw/index.js: not in list<br>- lib/v2/github/comments.js: asset |
The `lazyloadRouteHandler()` function isn't a Node.js primitive, so its not statically analyzable.

If you use `require.resolve()` then nft can analyze the module and its dependencies just like `require()`.

This will work as you expect:

```diff
- router.get('/1draw', lazyloadRouteHandler('./routes/1draw/index'));
+ router.get('/1draw', lazyloadRouteHandler(require.resolve('./routes/1draw/index')));
```

Note that `require.resolve()` will return the absolute path which you can then pass to `require()` dynamically.

Patch:
diff --git a/src/node-file-trace.ts b/src/node-file-trace.ts
--- a/src/node-file-trace.ts
+++ b/src/node-file-trace.ts
@@ -1,5 +1,5 @@
 import { NodeFileTraceOptions, NodeFileTraceResult, NodeFileTraceReasons, Stats, NodeFileTraceReasonType } from './types';
-import { basename, dirname, relative, resolve, sep } from 'path';
+import { basename, dirname, extname, relative, resolve, sep } from 'path';
 import fs from 'graceful-fs';
 import analyze, { AnalyzeResult } from './analyze';
 import resolveDependency from './resolve-dependency';
@@ -317,19 +317,12 @@ export class Job {
     if (path.endsWith('.json')) return;
     if (path.endsWith('.node')) return await sharedLibEmit(path, this);
 
-    const handlePjsonBoundary = async (item: string) => {
-      // js files require the "type": "module" lookup, so always emit the package.json
-      if (item.endsWith(".js")) {
-        const pjsonBoundary = await this.getPjsonBoundary(item);
-        if (pjsonBoundary)
-          await this.emitFile(
-            pjsonBoundary + sep + "package.json",
-            "resolve",
-            item
-          );
-      }
-    };
-    await handlePjsonBoundary(path);
+    // js files require the "type": "module" lookup, so always emit the package.json
+    if (path.endsWith('.js')) {
+      const pjsonBoundary = await this.getPjsonBoundary(path);
+      if (pjsonBoundary)
+        await this.emitFile(pjsonBoundary + sep + 'package.json', 'resolve', path);
+    }
 
     let analyzeResult: AnalyzeResult;
 
@@ -354,8 +347,12 @@ export class Job {
     
     await Promise.all([
       ...[...assets].map(async asset => {
-        await handlePjsonBoundary(asset);
-        await this.emitFile(asset, 'asset', path);
+        const ext = extname(asset);
+        if (ext === '.js' || ext === '.mjs' || ext === '.node' || ext === '' ||
+            this.ts && (ext === '.ts' || ext === '.tsx') && asset.startsWith(this.base) && asset.slice(this.base.length).indexOf(sep + 'node_modules' + sep) === -1)
+          await this.emitDependency(asset, path);
+        else
+          await this.emitFile(asset, 'asset', path);
       }),
       ...[...deps].map(async dep => {
         try {
diff --git a/src/utils/special-cases.ts b/src/utils/special-cases.ts
--- a/src/utils/special-cases.ts
+++ b/src/utils/special-cases.ts
@@ -6,7 +6,7 @@ import { Job } from '../node-file-trace';
 import { Ast } from './types';
 type Node = Ast['body'][0]
 
-const specialCases: Record<string, (o: SpecialCaseOpts) => Promise<void> | void> = {
+const specialCases: Record<string, (o: SpecialCaseOpts) => void> = {
   '@generated/photon' ({ id, emitAssetDirectory }) {
     if (id.endsWith('@generated/photon/index.js')) {
       emitAssetDirectory(resolve(dirname(id), 'runtime/'));


Test patch:
diff --git a/test/unit.test.js b/test/unit.test.js
--- a/test/unit.test.js
+++ b/test/unit.test.js
@@ -97,28 +97,6 @@ for (const { testName, isRoot } of unitTests) {
         isRoot ? join('./', unitPath, f) : join('test/unit', testName, f);
       
       const getReasonType = f => reasons.get(normalizeInputRoot(f)).type;
-
-      if (testName === 'fs-and-require') {
-        const dep1 = reasons.get(normalizeInputRoot('lib/dep1.js'));
-        expect(dep1).toBeDefined()
-        expect(dep1.parents).toEqual(new Set([normalizeInputRoot('input.js')]))
-        expect(dep1.type.sort()).toEqual(['asset', 'dependency'])
-
-        const dep2 = reasons.get(normalizeInputRoot('more/dep2.js'));
-        expect(dep2).toBeDefined()
-        expect(dep2.parents).toEqual(new Set([normalizeInputRoot('lib/dep1.js')]))
-        expect(dep2.type).toEqual(['dependency'])
-
-        const file1 = reasons.get(normalizeInputRoot('asset/file1.js'));
-        expect(file1).toBeDefined()
-        expect(file1.parents).toEqual(new Set([normalizeInputRoot('lib/dep1.js')]))
-        expect(file1.type).toEqual(['asset'])
-
-        const file2 = reasons.get(normalizeInputRoot('asset/file2.js'));
-        expect(file2).toBeDefined()
-        expect(file2.parents).toEqual(new Set([normalizeInputRoot('more/dep2.js')]))
-        expect(file2.type).toEqual(['asset'])
-      }
       
       if (testName === 'multi-input') {
         const collectFiles = (parent, files = new Set()) => {
diff --git a/test/unit/fs-and-require/asset/file1.js b/test/unit/fs-and-require/asset/file1.js
deleted file mode 100644
--- a/test/unit/fs-and-require/asset/file1.js
+++ /dev/null
@@ -1 +0,0 @@
-file1
\ No newline at end of file
diff --git a/test/unit/fs-and-require/asset/file2.js b/test/unit/fs-and-require/asset/file2.js
deleted file mode 100644
--- a/test/unit/fs-and-require/asset/file2.js
+++ /dev/null
@@ -1 +0,0 @@
-file2
\ No newline at end of file
diff --git a/test/unit/fs-and-require/input.js b/test/unit/fs-and-require/input.js
deleted file mode 100644
--- a/test/unit/fs-and-require/input.js
+++ /dev/null
@@ -1,9 +0,0 @@
-const { readdirSync } = require('fs');
-const { join } = require('path');
-
-readdirSync(join(__dirname, 'lib'))
-  .filter(f => f.endsWith('.js'))
-  .forEach(f => {
-    const mod = require('./lib/' + f);
-    console.log(`mod: ${mod}`);
-  });
\ No newline at end of file
diff --git a/test/unit/fs-and-require/lib/dep1.js b/test/unit/fs-and-require/lib/dep1.js
deleted file mode 100644
--- a/test/unit/fs-and-require/lib/dep1.js
+++ /dev/null
@@ -1,5 +0,0 @@
-const { join } = require('path');
-const file1 = join(__dirname, '..', 'asset', 'file1.js');
-const dep2 = require('../more/dep2.js');
-
-module.exports = `dep1 has asset ${file1} and dep ${dep2}`
diff --git a/test/unit/fs-and-require/more/dep2.js b/test/unit/fs-and-require/more/dep2.js
deleted file mode 100644
--- a/test/unit/fs-and-require/more/dep2.js
+++ /dev/null
@@ -1,4 +0,0 @@
-const { join } = require('path');
-const file2 = join(__dirname, '..', 'asset', 'file2.js');
-
-module.exports = `dep2 has file ${file2}`;
\ No newline at end of file
diff --git a/test/unit/fs-and-require/output.js b/test/unit/fs-and-require/output.js
deleted file mode 100644
--- a/test/unit/fs-and-require/output.js
+++ /dev/null
@@ -1,8 +0,0 @@
-[
-  "package.json",
-  "test/unit/fs-and-require/asset/file1.js",
-  "test/unit/fs-and-require/asset/file2.js",
-  "test/unit/fs-and-require/input.js",
-  "test/unit/fs-and-require/lib/dep1.js",
-  "test/unit/fs-and-require/more/dep2.js"
-]
\ No newline at end of file