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.

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

Merged
merged 8 commits into from
Feb 1, 2023

Conversation

ForsakenHarmony
Copy link
Member

@ForsakenHarmony ForsakenHarmony commented Jan 18, 2023

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

@vercel
Copy link

vercel bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
examples-cra-web 🔄 Building (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-nonmonorepo 🔄 Building (Inspect) Jan 31, 2023 at 4:38PM (UTC)
9 Ignored Deployments
Name Status Preview Comments Updated
examples-basic-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-designsystem-docs ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-tailwind-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)
turbo-site ⬜️ Ignored (Inspect) Visit Preview Jan 31, 2023 at 4:38PM (UTC)
turbo-vite-web ⬜️ Ignored (Inspect) Jan 31, 2023 at 4:38PM (UTC)

@ForsakenHarmony
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

🟢 CI successful 🟢

Thanks

}

#[turbo_tasks::value_trait]
pub trait ResolvePlugin {
Copy link
Contributor

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

pub async fn resolve(
we already have three clear passes:

  1. ImportMap(Resolver)
  2. Request(Resolver)
  3. FallbackImportMap(Resolver)

And so it seems like this would essentially add intermediate resolvers between 2. and 3.

Copy link
Member Author

@ForsakenHarmony ForsakenHarmony Jan 26, 2023

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)

Copy link
Contributor

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.

crates/next-core/src/next_server/resolve.rs Show resolved Hide resolved
crates/turbopack-core/src/resolve/plugin.rs Show resolved Hide resolved
crates/turbopack-core/src/resolve/plugin.rs Show resolved Hide resolved
crates/next-core/src/next_server/resolve.rs Show resolved Hide resolved
Comment on lines +64 to +85
// 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());
};
Copy link
Member

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.

Copy link
Member Author

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?

crates/turbopack-core/src/resolve/mod.rs Show resolved Hide resolved
@mehulkar mehulkar removed their request for review January 31, 2023 01:04
@ForsakenHarmony ForsakenHarmony removed the request for review from nathanhammond January 31, 2023 16:24
@ForsakenHarmony ForsakenHarmony removed the request for review from a team January 31, 2023 17:25
@github-actions
Copy link
Contributor

Benchmark for 4b9e04b

Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 7996.01µs ± 90.44µs 8933.10µs ± 110.25µs +11.72% +6.55%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9704.15µs ± 88.64µs 9036.33µs ± 179.35µs -6.88% -1.38%
bench_hmr_to_commit/Turbopack RSC/1000 modules 469.11ms ± 2.94ms 508.66ms ± 10.01ms +8.43% +2.87%
bench_startup/Turbopack RSC/1000 modules 2429.66ms ± 21.70ms 2374.19ms ± 5.04ms -2.28% -0.08%
bench_startup/Turbopack SSR/1000 modules 2026.28ms ± 9.01ms 1992.49ms ± 6.80ms -1.67% -0.11%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 7996.01µs ± 90.44µs 8933.10µs ± 110.25µs +11.72% +6.55%
bench_hmr_to_commit/Turbopack RCC/1000 modules 9704.15µs ± 88.64µs 9036.33µs ± 179.35µs -6.88% -1.38%
bench_hmr_to_commit/Turbopack RSC/1000 modules 469.11ms ± 2.94ms 508.66ms ± 10.01ms +8.43% +2.87%
bench_hmr_to_commit/Turbopack SSR/1000 modules 8389.87µs ± 152.98µs 8351.78µs ± 94.65µs -0.45%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8091.97µs ± 83.18µs 8042.63µs ± 83.30µs -0.61%
bench_hmr_to_eval/Turbopack RCC/1000 modules 8312.55µs ± 80.86µs 8174.45µs ± 73.11µs -1.66%
bench_hmr_to_eval/Turbopack SSR/1000 modules 8158.19µs ± 121.67µs 8419.77µs ± 95.07µs +3.21%
bench_hydration/Turbopack RCC/1000 modules 3698.28ms ± 21.08ms 3646.57ms ± 8.84ms -1.40%
bench_hydration/Turbopack RSC/1000 modules 3202.06ms ± 5.49ms 3208.25ms ± 11.28ms +0.19%
bench_hydration/Turbopack SSR/1000 modules 3034.85ms ± 16.24ms 3007.44ms ± 14.51ms -0.90%
bench_startup/Turbopack CSR/1000 modules 2010.86ms ± 7.31ms 2017.66ms ± 12.75ms +0.34%
bench_startup/Turbopack RCC/1000 modules 2526.44ms ± 10.51ms 2517.47ms ± 18.97ms -0.36%
bench_startup/Turbopack RSC/1000 modules 2429.66ms ± 21.70ms 2374.19ms ± 5.04ms -2.28% -0.08%
bench_startup/Turbopack SSR/1000 modules 2026.28ms ± 9.01ms 1992.49ms ± 6.80ms -1.67% -0.11%

@ForsakenHarmony ForsakenHarmony merged commit bc9e7c6 into main Feb 1, 2023
@ForsakenHarmony ForsakenHarmony deleted the hrmny/web-395-external-node_modules branch February 1, 2023 16:26
mehulkar pushed a commit that referenced this pull request Feb 1, 2023
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`
jridgewell pushed a commit to vercel/next.js that referenced this pull request Mar 10, 2023
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`
sokra pushed a commit to vercel/next.js that referenced this pull request Mar 13, 2023
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`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants