-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
external node_modules for SSR #3361
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
778d693
to
1e3a19d
Compare
0d30f91
to
7522b81
Compare
1e3a19d
to
262d934
Compare
262d934
to
9c34ee5
Compare
5800784
to
ba277c9
Compare
🟢 CI successful 🟢Thanks |
} | ||
|
||
#[turbo_tasks::value_trait] | ||
pub trait ResolvePlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a ResolvePlugin
or a straight up Resolver
? I'm asking because in https://github.com/vercel/turbo/blob/89ead9b9d4a78b27c3eb100b4de580ed7ddd82c0/crates/turbopack-core/src/resolve/mod.rs#L576 we already have three clear passes:
ImportMap(Resolver)
Request(Resolver)
FallbackImportMap(Resolver)
And so it seems like this would essentially add intermediate resolvers between 2. and 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand the question, what's a Resolver
? Would that be a new type?
To me, the resolve
function is the Resolver
The ResolvePlugin
right now is kinda part of pass 2, we already have the resolved_map
in there.
And it would later on also become part of pass 1
Was thinking the import map could be implemented as a ResolvePlugin
(not that we would necessarily want that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolver
is just another name for ResolvePlugin
. I'm saying the current resolving logic can be separated into 3 ResolvePlugins
.
ba277c9
to
6dd5f55
Compare
// make sure we have a full package | ||
let Some(package_name) = package["name"].as_str() else { | ||
return Ok(ResolveResultOptionVc::none()); | ||
}; | ||
|
||
// check if we can resolve the package from the root dir (might be hidden by | ||
// pnpm) | ||
let FileJsonContent::Content(resolved_package) = &*self | ||
.root | ||
.join(&format!("node_modules/{}/package.json", package_name)) | ||
.read_json() | ||
.await? | ||
else { | ||
return Ok(ResolveResultOptionVc::none()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit weird and also doesn't support to make packages external that have a inner package.json.
Could we try to resolve the request
from the project_path
and from the current context
(need to passed to plugin) with some special ResolveOptions
that match node.js behavior? And if both resolve to the same path and that isn't a ESM we can use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't get the original request here, so that's hard to do
Are there any cjs modules that have an inner package.json?
6dd5f55
to
469b938
Compare
48ec02a
to
8a99490
Compare
8a99490
to
e0430b2
Compare
Benchmark for 4b9e04b
Click to view full benchmark
|
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
…49736102+kodiakhq[bot]@users.noreply.github.com> # New Features - vercel/turborepo#3540 - vercel/turborepo#3549 - vercel/turborepo#3465 - vercel/turborepo#3550 - vercel/turborepo#3495 - vercel/turborepo#3624 - vercel/turborepo#3600 - vercel/turborepo#3676 - vercel/turborepo#3689 # Fixes - vercel/turborepo#3437 - vercel/turborepo#3542 - vercel/turborepo#3531 - vercel/turborepo#3552 - vercel/turborepo#3551 - vercel/turborepo#3597 - vercel/turborepo#3644 - vercel/turborepo#3623 - vercel/turborepo#3634 - vercel/turborepo#3574 - vercel/turborepo#3673 - vercel/turborepo#3675 - vercel/turborepo#3723 - vercel/turborepo#3677 - vercel/turborepo#3717 - vercel/turborepo#3701 # Performance Improvements - vercel/turborepo#3361 - vercel/turborepo#3619 --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
Adds resolve plugins (currently only running `after_resolve`, could add `resolve` later) Had to fix `ResolveResult::merge_alternatives` because it merged two external results into `Unresolvable`
Adds resolve plugins (currently only running
after_resolve
, could addresolve
later)Had to fix
ResolveResult::merge_alternatives
because it merged two external results intoUnresolvable