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.

Sign up for GitHub

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

fix: add parent node_modules directories #136

Closed
wants to merge 2 commits into from
Closed

fix: add parent node_modules directories #136

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2868b8b
fix: add parent node_modules directories
antfu Apr 21, 2022
dd34fc0
chore: update
antfu Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { resolve } from 'pathe'
import { existsSync } from 'fs'
import { dirname, join, resolve } from 'pathe'
import { loadConfig } from 'c12'
import { klona } from 'klona/full'
import { camelCase } from 'scule'
Expand Down Expand Up @@ -122,7 +123,15 @@ export async function loadOptions (userConfig: NitroConfig = {}): Promise<NitroO
options.output.publicDir = resolvePath(options.output.publicDir, options)
options.output.serverDir = resolvePath(options.output.serverDir, options)

options.nodeModulesDirs.push(resolve(options.rootDir, 'node_modules'))
// Scan and add parent node_modules directory
let currentDir = resolve(options.rootDir)
while (currentDir && currentDir !== dirname(currentDir)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Node.js search algorithm, this should normally happen when all parent node_modules are scanned. (https://nodejs.org/api/modules.html#modules_all_together see NODE_MODULES_PATHS section). Might this be an issue with rollup plugin?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this PR is trying to mimic the behavior of Node. The bug of https://github.com/nuxt/framework/issues/4217 is that the dependencies are hoisted to the upper level so the rollup failed to find it. To me, it sounds reasonable for rollup to only scan the node_modules when you provide them explicitly. I think this is quite an important bug, maybe we could have it for now and raise the discussion to rollup later?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not confident to add this and masking issue without a minimal reproduction. We use plugin node-resolve that internally uses resolve package. They are supposed to do exactly this!

https://github.com/browserify/resolve/blob/dc8ad1945c663ba47821a269aefb5549f6177cf7/lib/node-modules-paths.js#L40

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to reproduce. Can you please check this if can find a way to reproduce? https://github.com/pi0/nitro-reproduction

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/antfu/nuxt-monorepo-repro

cd nuxt-app and npm run build

We had the same issue in the modules.nuxtjs.org migration here

https://vercel.com/nuxt-js/modules/5TEN9v28ppMEzdtFz5whmsnRKKcc

and workaround as
nuxt/modules@e2ac251 (#357)

With the suggestions https://github.com/nuxt/framework/issues/4217#issuecomment-1094852657 by @danielroe

Copy link
Member

@pi0 pi0 Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for repro but my a minimal reproduction for nitro, I meant a nitro project. I've updated mine with yarn 3 monorepo however this issue doesn't seems to exist. It is something with nuxt integration or vue alias.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated your repro. By cd app && yarn build

Copy link
Member

@pi0 pi0 Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Now i can reproduce it. (More precisely with externals: { inline: ['lodash-es'] })

const nodeModule = join(currentDir, 'node_modules')
if (existsSync(nodeModule)) {
options.nodeModulesDirs.push(nodeModule)
}
currentDir = dirname(currentDir)
}
options.nodeModulesDirs.push(resolve(pkgDir, 'node_modules'))
options.nodeModulesDirs = Array.from(new Set(options.nodeModulesDirs))

Expand Down