Apply serverExternalPackages inside node_modules and add keyv (#67948)

ExternalCjsModulesResolvePlugin was originally disabled for requests inside node_modules (e.g. from inside `cacheable-request` to `keyv`) by @sokra in vercel/turborepo#3736. But it seems that we do want this to happen In the `ExternalPredicate::AllExcept` case, requests inside node_modules are still ignored. Otherwise `next/dist/compiled/next-server/pages.runtime.dev.js` was marked as external and failed many tests With that fixed, add `keyv` so that `got` can be imported. Fixes #67282 Fixes PACK-3139
vercel · Jul 25, 2024 · 16a55ae · 16a55ae
1 parent 29c23b2
commit 16a55ae
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 12 deletions.
diff --git a/crates/next-core/src/next_server/resolve.rs b/crates/next-core/src/next_server/resolve.rs
@@ -24,8 +24,10 @@ use turbopack_binding::{
 #[turbo_tasks::value(into = "shared")]
 pub enum ExternalPredicate {
     /// Mark all modules as external if they're not listed in the list.
+    /// Applies only to imports outside of node_modules.
     AllExcept(Vc<Vec<RcStr>>),
-    /// Only mark modules listed as external.
+    /// Only mark modules listed as external, whether inside node_modules or
+    /// not.
     Only(Vc<Vec<RcStr>>),
 }
 
@@ -76,13 +78,10 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
     async fn after_resolve(
         &self,
         fs_path: Vc<FileSystemPath>,
-        context: Vc<FileSystemPath>,
+        lookup_path: Vc<FileSystemPath>,
         reference_type: Value<ReferenceType>,
         request: Vc<Request>,
     ) -> Result<Vc<ResolveResultOption>> {
-        if *condition(self.root).matches(context).await? {
-            return Ok(ResolveResultOption::none());
-        }
         let request_value = &*request.await?;
         if !matches!(request_value, Request::Module { .. }) {
             return Ok(ResolveResultOption::none());
@@ -104,6 +103,10 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
         let predicate = self.predicate.await?;
         let must_be_external = match &*predicate {
             ExternalPredicate::AllExcept(exceptions) => {
+                if *condition(self.root).matches(lookup_path).await? {
+                    return Ok(ResolveResultOption::none());
+                }
+
                 let exception_glob = packages_glob(*exceptions).await?;
 
                 if let Some(PackagesGlobs {
@@ -205,13 +208,13 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
         let mut request = request;
 
         let node_resolve_options = if is_esm {
-            node_esm_resolve_options(context.root())
+            node_esm_resolve_options(lookup_path.root())
         } else {
-            node_cjs_resolve_options(context.root())
+            node_cjs_resolve_options(lookup_path.root())
         };
         let result_from_original_location = loop {
             let node_resolved_from_original_location = resolve(
-                context,
+                lookup_path,
                 reference_type.clone(),
                 request,
                 node_resolve_options,
@@ -367,7 +370,7 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
                 // It would be more efficient to use an CJS external instead of an ESM external,
                 // but we need to verify if that would be correct (as in resolves to the same
                 // file).
-                let node_resolve_options = node_cjs_resolve_options(context.root());
+                let node_resolve_options = node_cjs_resolve_options(lookup_path.root());
                 let node_resolved = resolve(
                     self.project_path,
                     reference_type.clone(),

diff --git a/crates/next-core/src/next_shared/resolve.rs b/crates/next-core/src/next_shared/resolve.rs
@@ -232,7 +232,7 @@ impl AfterResolvePlugin for NextExternalResolvePlugin {
     async fn after_resolve(
         &self,
         fs_path: Vc<FileSystemPath>,
-        _context: Vc<FileSystemPath>,
+        _lookup_path: Vc<FileSystemPath>,
         _reference_type: Value<ReferenceType>,
         _request: Vc<Request>,
     ) -> Result<Vc<ResolveResultOption>> {
@@ -282,7 +282,7 @@ impl AfterResolvePlugin for NextNodeSharedRuntimeResolvePlugin {
     async fn after_resolve(
         &self,
         fs_path: Vc<FileSystemPath>,
-        _context: Vc<FileSystemPath>,
+        _lookup_path: Vc<FileSystemPath>,
         _reference_type: Value<ReferenceType>,
         _request: Vc<Request>,
     ) -> Result<Vc<ResolveResultOption>> {
@@ -406,7 +406,7 @@ impl AfterResolvePlugin for NextSharedRuntimeResolvePlugin {
     async fn after_resolve(
         &self,
         fs_path: Vc<FileSystemPath>,
-        _context: Vc<FileSystemPath>,
+        _lookup_path: Vc<FileSystemPath>,
         _reference_type: Value<ReferenceType>,
         _request: Vc<Request>,
     ) -> Result<Vc<ResolveResultOption>> {

diff --git a/docs/02-app/02-api-reference/05-next-config-js/serverExternalPackages.mdx b/docs/02-app/02-api-reference/05-next-config-js/serverExternalPackages.mdx
@@ -47,6 +47,7 @@ Next.js includes a [short list of popular packages](https://github.com/vercel/ne
 - `isolated-vm`
 - `jest`
 - `jsdom`
+- `keyv`
 - `libsql`
 - `mdx-bundler`
 - `mongodb`

diff --git a/docs/03-pages/02-api-reference/03-next-config-js/serverExternalPackages.mdx b/docs/03-pages/02-api-reference/03-next-config-js/serverExternalPackages.mdx
@@ -47,6 +47,7 @@ Next.js includes a [short list of popular packages](https://github.com/vercel/ne
 - `isolated-vm`
 - `jest`
 - `jsdom`
+- `keyv`
 - `libsql`
 - `mdx-bundler`
 - `mongodb`

diff --git a/packages/next/src/lib/server-external-packages.json b/packages/next/src/lib/server-external-packages.json
@@ -28,6 +28,7 @@
   "isolated-vm",
   "jest",
   "jsdom",
+  "keyv",
   "libsql",
   "mdx-bundler",
   "mongodb",