Skip to content

Commit

Permalink
Turbopack: Improve externals plugin (vercel#71375)
Browse files Browse the repository at this point in the history 
- Slightly improve externals error message
- Don't try to append `.js` to requests without subpaths
- Simplify request->package name parsing

Before 
![Bildschirmfoto 2024-10-16 um 14 27
05](https://github.com/user-attachments/assets/1308084e-27cb-4d77-a472-b202736acc3b)

After (app code changed to include a subpath for better visualization)
<img width="963" alt="Bildschirmfoto 2024-10-17 um 11 24 39"
src="https://github.com/user-attachments/assets/f2d5caa7-a2f4-498b-88aa-a026aedcabe5">


Without the CSS change, it can look like this if there is no `-` in the
filepath:

<img width="1005" alt="Bildschirmfoto 2024-10-17 um 10 55 13"
src="https://github.com/user-attachments/assets/1a899d46-87cc-4e32-8005-92232fee318f">
  • Loading branch information
@mischnic
mischnic authored Oct 18, 2024
1 parent 3a9a1df commit 5c404e9
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 89 deletions.
175 changes: 86 additions & 89 deletions crates/next-core/src/next_server/resolve.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use turbopack_core::{
node::{node_cjs_resolve_options, node_esm_resolve_options},
package_json,
parse::Request,
pattern::Pattern,
plugin::{AfterResolvePlugin, AfterResolvePluginCondition},
resolve, ExternalType, FindContextFileResult, ResolveResult, ResolveResultItem,
ResolveResultOption,
Expand Down Expand Up @@ -81,17 +82,22 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
request: Vc<Request>,
) -> Result<Vc<ResolveResultOption>> {
let request_value = &*request.await?;
if !matches!(request_value, Request::Module { .. }) {
let Request::Module {
module: package,
path: package_subpath,
..
} = request_value
else {
return Ok(ResolveResultOption::none());
}
};

// from https://github.com/vercel/next.js/blob/8d1c619ad650f5d147207f267441caf12acd91d1/packages/next/src/build/handle-externals.ts#L188
let never_external_regex = lazy_regex::regex!("^(?:private-next-pages\\/|next\\/(?:dist\\/pages\\/|(?:app|document|link|image|legacy\\/image|constants|dynamic|script|navigation|headers|router)$)|string-hash|private-next-rsc-action-validate|private-next-rsc-action-client-wrapper|private-next-rsc-server-reference$)");

let request_str = request_value.request().map(|v| v.into_owned());
let Some(mut request_str) = request_str else {
let Pattern::Constant(package_subpath) = package_subpath else {
return Ok(ResolveResultOption::none());
};
let request_str: RcStr = format!("{package}{package_subpath}").into();
if never_external_regex.is_match(&request_str) {
return Ok(ResolveResultOption::none());
}
Expand Down Expand Up @@ -190,12 +196,13 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
Ok(FileType::UnsupportedExtension)
}

let unable_to_externalize = |request_str: RcStr, reason: &str| {
let unable_to_externalize = |reason: Vec<StyledString>| {
if must_be_external {
UnableToExternalize {
file_path: fs_path,
request: request_str,
reason: reason.into(),
ExternalizeIssue {
file_path: lookup_path,
package: package.clone(),
request_str: request_str.clone(),
reason,
}
.cell()
.emit();
Expand All @@ -204,6 +211,7 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
};

let mut request = request;
let mut request_str = request_str.to_string();

let node_resolve_options = if is_esm {
node_esm_resolve_options(lookup_path.root())
Expand All @@ -220,7 +228,11 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
let Some(result_from_original_location) =
*node_resolved_from_original_location.first_source().await?
else {
if is_esm && !request_str.ends_with(".js") {
if is_esm
&& package_subpath != ""
&& package_subpath != "/"
&& !request_str.ends_with(".js")
{
// We have a fallback solution for convinience: If user doesn't
// have an extension in the request we try to append ".js"
// automatically
Expand All @@ -229,14 +241,14 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
continue;
}
// this can't resolve with node.js from the original location, so bundle it
return unable_to_externalize(
request_str.into(),
return unable_to_externalize(vec![StyledString::Text(
"The request could not be resolved by Node.js from the importing module. The \
way Node.js resolves modules is slightly different from the way Next.js \
resolves modules. Next.js was able to resolve it, while Node.js would not be \
able to.\nTry to remove this package from serverExternalPackages.\nOr update \
the import side to use a compatible request that can be resolved by Node.js.",
);
the import side to use a compatible request that can be resolved by Node.js."
.into(),
)]);
};
break result_from_original_location;
};
Expand All @@ -249,13 +261,17 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {

let Some(result) = *node_resolved.first_source().await? else {
// this can't resolve with node.js from the project directory, so bundle it
return unable_to_externalize(
request_str.into(),
"The request could not be resolved by Node.js from the project \
directory.\nPackages that should be external need to be installed in the project \
directory, so they can be resolved from the output files.\nTry to install the \
package into the project directory.",
);
return unable_to_externalize(vec![
StyledString::Text(
"The request could not be resolved by Node.js from the project \
directory.\nPackages that should be external need to be installed in the \
project directory, so they can be resolved from the output files.\nTry to \
install it into the project directory by running "
.into(),
),
StyledString::Code(format!("npm install {package}").into()),
StyledString::Text(" from the project directory.".into()),
]);
};

let result = result.resolve().await?;
Expand All @@ -276,45 +292,42 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
);
let FindContextFileResult::Found(package_json_file, _) = *package_json_file.await?
else {
return unable_to_externalize(
request_str.into(),
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package resolved from the project directory can't be \
found.",
);
found."
.into(),
)]);
};
let FindContextFileResult::Found(package_json_from_original_location, _) =
*package_json_from_original_location.await?
else {
return unable_to_externalize(
request_str.into(),
"The package.json of the package can't be found.",
);
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package can't be found.".into(),
)]);
};
let FileJsonContent::Content(package_json_file) =
&*package_json_file.read_json().await?
else {
return unable_to_externalize(
request_str.into(),
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package resolved from project directory can't be \
parsed.",
);
parsed."
.into(),
)]);
};
let FileJsonContent::Content(package_json_from_original_location) =
&*package_json_from_original_location.read_json().await?
else {
return unable_to_externalize(
request_str.into(),
"The package.json of the package can't be parsed.",
);
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package can't be parsed.".into(),
)]);
};
let (Some(name), Some(version)) = (
package_json_file.get("name").and_then(|v| v.as_str()),
package_json_file.get("version").and_then(|v| v.as_str()),
) else {
return unable_to_externalize(
request_str.into(),
"The package.json of the package has no name or version.",
);
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package has no name or version.".into(),
)]);
};
let (Some(name2), Some(version2)) = (
package_json_from_original_location
Expand All @@ -324,23 +337,23 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
.get("version")
.and_then(|v| v.as_str()),
) else {
return unable_to_externalize(
request_str.into(),
return unable_to_externalize(vec![StyledString::Text(
"The package.json of the package resolved from project directory has no name \
or version.",
);
or version."
.into(),
)]);
};
if (name, version) != (name2, version2) {
// this can't resolve with node.js from the original location, so bundle it
return unable_to_externalize(
request_str.into(),
&format!(
return unable_to_externalize(vec![StyledString::Text(
format!(
"The package resolves to a different version when requested from the \
project directory ({version}) compared to the package requested from the \
importing module ({version2}).\nMake sure to install the same version of \
the package in both locations."
),
);
)
.into(),
)]);
}
}
let path = result.ident().path().resolve().await?;
Expand All @@ -349,17 +362,15 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
match (file_type, is_esm) {
(FileType::UnsupportedExtension, _) => {
// unsupported file type, bundle it
unable_to_externalize(
request_str.into(),
"Only .mjs, .cjs, .js, .json, or .node can be handled by Node.js.",
)
unable_to_externalize(vec![StyledString::Text(
"Only .mjs, .cjs, .js, .json, or .node can be handled by Node.js.".into(),
)])
}
(FileType::InvalidPackageJson, _) => {
// invalid package.json, bundle it
unable_to_externalize(
request_str.into(),
"The package.json can't be found or parsed.",
)
unable_to_externalize(vec![StyledString::Text(
"The package.json can't be found or parsed.".into(),
)])
}
(FileType::CommonJs, false) => {
// mark as external
Expand Down Expand Up @@ -426,11 +437,11 @@ impl AfterResolvePlugin for ExternalCjsModulesResolvePlugin {
(FileType::EcmaScriptModule, false) => {
// even with require() this resolves to a ESM,
// which would break node.js, bundle it
unable_to_externalize(
request_str.into(),
unable_to_externalize(vec![StyledString::Text(
"The package seems invalid. require() resolves to a EcmaScript module, which \
would result in an error in Node.js.",
)
would result in an error in Node.js."
.into(),
)])
}
}
}
Expand Down Expand Up @@ -462,40 +473,26 @@ async fn packages_glob(packages: Vc<Vec<RcStr>>) -> Result<Vc<OptionPackagesGlob
}

#[turbo_tasks::value]
struct UnableToExternalize {
struct ExternalizeIssue {
file_path: Vc<FileSystemPath>,
request: RcStr,
reason: RcStr,
package: RcStr,
request_str: RcStr,
reason: Vec<StyledString>,
}

#[turbo_tasks::value_impl]
impl Issue for UnableToExternalize {
impl Issue for ExternalizeIssue {
#[turbo_tasks::function]
fn severity(&self) -> Vc<IssueSeverity> {
IssueSeverity::Warning.cell()
}

#[turbo_tasks::function]
async fn title(&self) -> Vc<StyledString> {
let request = &self.request;
let package = if request.starts_with('@') {
request
.splitn(3, '/')
.take(2)
.intersperse("/")
.collect::<String>()
.into()
} else if let Some((package, _)) = request.split_once('/') {
package.into()
} else {
request.clone()
};
StyledString::Line(vec![
StyledString::Text("Package ".into()),
StyledString::Code(package),
StyledString::Text(" (".into()),
StyledString::Code("serverExternalPackages".into()),
StyledString::Text(" or default list) can't be external".into()),
StyledString::Code(self.package.clone()),
StyledString::Text(" can't be external".into()),
])
.cell()
}
Expand All @@ -511,19 +508,19 @@ impl Issue for UnableToExternalize {
}

#[turbo_tasks::function]
fn description(&self) -> Vc<OptionStyledString> {
Vc::cell(Some(
async fn description(&self) -> Result<Vc<OptionStyledString>> {
Ok(Vc::cell(Some(
StyledString::Stack(vec![
StyledString::Line(vec![
StyledString::Text("The request ".into()),
StyledString::Code(self.request.clone()),
StyledString::Code(self.request_str.clone()),
StyledString::Text(" matches ".into()),
StyledString::Code("serverExternalPackages".into()),
StyledString::Text(" (or the default list), but it can't be external:".into()),
StyledString::Text(" (or the default list).".into()),
]),
StyledString::Line(vec![StyledString::Text(self.reason.clone())]),
StyledString::Line(self.reason.clone()),
])
.cell(),
))
)))
}
}
1 change: 1 addition & 0 deletions ...ages/next/src/client/components/react-dev-overlay/internal/components/Terminal/styles.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ const styles = css`
display: flex;
align-items: center;
justify-content: space-between;
line-break: anywhere;
}
[data-with-open-in-editor-link-import-trace] {
margin-left: var(--size-gap-double);
Expand Down

0 comments on commit 5c404e9

Please sign in to comment.