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

Prepare RouterContentSource for basePath #5218

Merged
merged 3 commits into from
Jun 6, 2023
Merged

Prepare RouterContentSource for basePath #5218

merged 3 commits into from
Jun 6, 2023

Conversation

jridgewell
Copy link
Contributor

Description

This is part 2 of WEB-993 basePath support. A few of our next-specific content sources will need to be scoped under the basePath (like _next/image and __nextjs_original-stack-frame). These are currently served with a RouterContentSource, but it didn't have support for arbitrary prefixes.

We could have changed the subpath for these sources to include the basePath, but that would require reading the next_config.base_path() in the source method, and it would invalidate our entire call graph whenever the next.config.js changed. Not a good choice.

Testing Instructions

@jridgewell jridgewell requested a review from a team as a code owner June 6, 2023 02:57
@vercel
Copy link

vercel bot commented Jun 6, 2023

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

Name Status Preview Comments Updated (UTC)
examples-basic-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-cra-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-designsystem-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-gatsby-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-kitchensink-blog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-native-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-svelte-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-tailwind-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
examples-vite-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 3:50pm

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

✅ This change can build next-swc

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Linux Benchmark for adb3170

Click to view benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 8891.94µs ± 75.33µs 8871.24µs ± 47.07µs -0.23%
bench_hmr_to_eval/Turbopack CSR/1000 modules 7996.58µs ± 78.74µs 8279.71µs ± 149.88µs +3.54%
bench_startup/Turbopack CSR/1000 modules 915.08ms ± 2.02ms 907.74ms ± 3.90ms -0.80%

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

MacOS Benchmark for adb3170

Test Base PR % Significant %
bench_hmr_to_eval/Turbopack CSR/1000 modules 59.38ms ± 3.51ms 25.51ms ± 0.17ms -57.03% -50.63%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 26.20ms ± 0.27ms 29.16ms ± 1.95ms +11.28%
bench_hmr_to_eval/Turbopack CSR/1000 modules 59.38ms ± 3.51ms 25.51ms ± 0.17ms -57.03% -50.63%
bench_startup/Turbopack CSR/1000 modules 3135.01ms ± 50.36ms 3088.91ms ± 79.08ms -1.47%

Comment on lines +36 to +40
if cfg!(debug_assertions) {
let prefix_string = prefix.await?;
debug_assert!(prefix_string.is_empty() || prefix_string.ends_with('/'));
debug_assert!(prefix_string.starts_with('/'));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be achieved with a BasePath newtype (BasePathVc), which would make these assertions in its String -> BasePath constructor.

But I don't think the newtype Vc story is good enough to warrant this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could generalize this to a PathStr type, which would have match these same rules. All ContentSource's would receive this as the path param. PrefixedRouterContentSource and StaticAssetsContentSource could receive them as the prefix as well.

crates/turbopack-dev-server/src/source/router.rs Outdated Show resolved Hide resolved
crates/turbopack-dev-server/src/source/router.rs Outdated Show resolved Hide resolved
crates/turbopack-dev-server/src/source/static_assets.rs Outdated Show resolved Hide resolved
crates/turbopack-dev-server/src/source/router.rs Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Linux Benchmark for 53f59af

Test Base PR % Significant %
bench_hmr_to_eval/Turbopack CSR/1000 modules 8961.23µs ± 45.73µs 9327.58µs ± 70.55µs +4.09% +1.48%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 10.21ms ± 0.06ms 10.06ms ± 0.09ms -1.47%
bench_hmr_to_eval/Turbopack CSR/1000 modules 8961.23µs ± 45.73µs 9327.58µs ± 70.55µs +4.09% +1.48%
bench_startup/Turbopack CSR/1000 modules 948.77ms ± 1.59ms 943.02ms ± 4.98ms -0.61%

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

MacOS Benchmark for 53f59af

Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 28.80ms ± 1.07ms 25.06ms ± 0.21ms -12.98% -4.43%
Click to view full benchmark
Test Base PR % Significant %
bench_hmr_to_commit/Turbopack CSR/1000 modules 28.80ms ± 1.07ms 25.06ms ± 0.21ms -12.98% -4.43%
bench_hmr_to_eval/Turbopack CSR/1000 modules 34.56ms ± 4.67ms 48.59ms ± 3.80ms +40.59%
bench_startup/Turbopack CSR/1000 modules 3159.16ms ± 164.50ms 2999.73ms ± 89.55ms -5.05%

ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

This is part 2 of [WEB-993](https://linear.app/vercel/issue/WEB-993)
basePath support. A few of our next-specific content sources will need
to be scoped under the `basePath` (like `_next/image` and
`__nextjs_original-stack-frame`). These are currently served with a
`RouterContentSource`, but it didn't have support for arbitrary
prefixes.

We _could_ have changed the subpath for these sources to include the
`basePath`, but that would require reading the `next_config.base_path()`
in the
[source](https://github.com/vercel/next.js/blob/2b1f0d9351610b04d01638efed19252ca81d0023/packages/next-swc/crates/next-dev/src/lib.rs#L413-L423)
method, and it would invalidate our entire call graph whenever the
`next.config.js` changed. Not a good choice.


### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

This is part 2 of [WEB-993](https://linear.app/vercel/issue/WEB-993)
basePath support. A few of our next-specific content sources will need
to be scoped under the `basePath` (like `_next/image` and
`__nextjs_original-stack-frame`). These are currently served with a
`RouterContentSource`, but it didn't have support for arbitrary
prefixes.

We _could_ have changed the subpath for these sources to include the
`basePath`, but that would require reading the `next_config.base_path()`
in the
[source](https://github.com/vercel/next.js/blob/2b1f0d9351610b04d01638efed19252ca81d0023/packages/next-swc/crates/next-dev/src/lib.rs#L413-L423)
method, and it would invalidate our entire call graph whenever the
`next.config.js` changed. Not a good choice.


### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

This is part 2 of [WEB-993](https://linear.app/vercel/issue/WEB-993)
basePath support. A few of our next-specific content sources will need
to be scoped under the `basePath` (like `_next/image` and
`__nextjs_original-stack-frame`). These are currently served with a
`RouterContentSource`, but it didn't have support for arbitrary
prefixes.

We _could_ have changed the subpath for these sources to include the
`basePath`, but that would require reading the `next_config.base_path()`
in the
[source](https://github.com/vercel/next.js/blob/2b1f0d9351610b04d01638efed19252ca81d0023/packages/next-swc/crates/next-dev/src/lib.rs#L413-L423)
method, and it would invalidate our entire call graph whenever the
`next.config.js` changed. Not a good choice.


### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: automerge Kodiak will merge these automatically after checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants