fix(turbopack-ecmascript): support urlrewritebehavior::full #6413

kwonoj · 2023-11-09T18:09:07Z

Description

This PR tries to correct the behavior of UrlRewriteBehavior::Full.

With UrlRewriteBehavior::Full, it is expected new URL(url, base) resolves to the absolute full path to the requested url. Previously, this didn't work for 2 reasons

replacing url to the emitted output path
replacing base to non-relative to the above output path

in result resolving won't give the correct path.

I tried a couple of approaches if it's possible to construct full path directly when there's a referenced asset, but wasn't able to make it work. Instead this PR relies on the new runtime fn wraps __turbopack_require__, calculate the full path with the absolute path runtime already have + resolved asset path. Since emitted asset contains its output with assetPrefix, exports it into the runtime as well to strips it out.

Overall behaviors across different condition are illustarted here:

https://github.com/vercel/turbo/compare/url-full-resolve-context?expand=1#diff-bb24150ae91f0fa7b3001d17e597f89d3e16f5e0f4894acb4bd0499fbcd73793R134

Closes PACK-1924

vercel · 2023-11-09T18:09:21Z

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

11 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-cra-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-nonmonorepo	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-svelte-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Nov 17, 2023 1:15am

github-actions · 2023-11-09T18:14:24Z

✅ This change can build next-swc

github-actions · 2023-11-09T18:18:04Z

🟢 CI successful 🟢

Thanks

github-actions · 2023-11-09T18:43:08Z

Linux Benchmark for `10232f6`

Click to view benchmark

Test	Base	PR	%
bench_hmr_to_commit/Turbopack CSR/1000 modules	21.68ms ± 0.85ms	21.81ms ± 0.89ms	+0.63%
bench_hmr_to_eval/Turbopack CSR/1000 modules	21.39ms ± 0.85ms	21.29ms ± 0.84ms	-0.46%
bench_startup/Turbopack CSR/1000 modules	1165.59ms ± 7.98ms	1178.26ms ± 19.42ms	+1.09%

github-actions · 2023-11-09T18:48:04Z

⚠️ Turbopack Benchmark CI failed ⚠️

The following steps have failed in CI:

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

See workflow summary for details

crates/turbopack-ecmascript-runtime/js/src/build/runtime.ts

crates/turbopack-ecmascript/src/references/esm/url.rs

jridgewell · 2023-11-09T22:08:19Z

crates/turbopack-ecmascript/src/references/esm/url.rs

+    ├───────────────────────────────┼─────────────────────────────────────────────────────────────────────────┼────────────────────────────────────────────────┼───────────────────────┤
+    │ Relative                      │ new URL(__turbopack_relative_url__(__turbopack_require__(urlId)), base) │ new URL(__turbopack_relative_url__(url), base) │ new URL(url, base)    │
+    │ Full(RenderingClient::Client) │ new URL(__turbopack_require__(urlId), "location.origin")                │ new URL(url, "location.origin")                │ new URL(url, base)    │
+    │ Full(RenderingClient::..)     │ new URL(__turbopack_resolve_module_id_path__(urlId))                    │ new URL(url, base)                             │ new URL(url, base)    │


Why do we need another module wrapper function for this? Why can't we derive this from CWD or similar?

We have cwd, but that's not composable with relative path url to its output asset.

The simpliest example is like new URL('../../public/img.png', import.meta.url): we have correct import.meta.url points to yhe cwd of the module, but _turbopack_require'd module points to the output distdir's asset with asset prefix so can't relate those two directlt.

This is the actual problem I struggled to solve without these wrapper, if there's a way to get a full phsyical path without asset prefix it'd be great. I feel I may miss something.

crates/turbopack-ecmascript/src/references/esm/url.rs

github-actions · 2023-11-13T17:29:17Z

Linux Benchmark for `b30a55a`

Click to view benchmark

Test	Base	PR	%
bench_hmr_to_commit/Turbopack CSR/1000 modules	23.23ms ± 0.82ms	22.79ms ± 0.97ms	-1.91%
bench_hmr_to_eval/Turbopack CSR/1000 modules	22.59ms ± 1.00ms	22.48ms ± 0.97ms	-0.46%
bench_startup/Turbopack CSR/1000 modules	1170.79ms ± 5.61ms	1163.40ms ± 7.58ms	-0.63%

crates/turbopack-ecmascript/src/references/esm/url.rs

rustc-ice-2023-11-13T17:05:44.397158Z-94125.txt

sokra

you can add an execution turbopack-test to test this

github-actions · 2023-11-13T22:05:13Z

Linux Benchmark for 0410e52

Click to view benchmark

Test	Base	PR	%
bench_hmr_to_commit/Turbopack CSR/1000 modules	22.53ms ± 0.95ms	22.51ms ± 0.95ms	-0.12%
bench_hmr_to_eval/Turbopack CSR/1000 modules	22.24ms ± 0.83ms	21.97ms ± 0.89ms	-1.20%
bench_startup/Turbopack CSR/1000 modules	1158.85ms ± 2.41ms	1154.94ms ± 10.77ms	-0.34%

kwonoj · 2023-11-14T17:04:52Z

@sokra

you can add an execution turbopack-test to test this

Gave a quick peek, and then realized test uses different runtime to the runtime next-swc's turbopack uses, which doesn't have any foundation support such as runtime's absolute path, etcs. I'd like to tackle this as separate PR later as have to port runtime's feature other than my changes.

sokra · 2023-11-14T17:08:31Z

Gave a quick peek, and then realized test uses different runtime to the runtime next-swc's turbopack uses, which doesn't have any foundation support such as runtime's absolute path, etcs. I'd like to tackle this as separate PR later as have to port runtime's feature other than my changes.

Fine to me, at least run it against the next.js test case to verify it's working

kwonoj · 2023-11-14T17:10:04Z

Fine to me, at least run it against the next.js test case to verify it's working

Yes, as shared in the slack thread vercel/next.js#58404 verifies new url tests are passing (vercel/next.js@4e63956#diff-2731c6687251176e46f1aa219fe2c059c9a13b61f0a5035d9b5073c9fc10258e) . Failed test is flaky I believe.

github-actions · 2023-11-14T17:37:37Z

Linux Benchmark for `9c4b9bc`

Click to view benchmark

Test	Base	PR	%
bench_hmr_to_commit/Turbopack CSR/1000 modules	21.70ms ± 0.86ms	21.77ms ± 0.86ms	+0.32%
bench_hmr_to_eval/Turbopack CSR/1000 modules	21.28ms ± 0.84ms	21.43ms ± 0.87ms	+0.68%
bench_startup/Turbopack CSR/1000 modules	1155.75ms ± 4.46ms	1145.05ms ± 7.92ms	-0.93%

kwonoj requested a review from a team as a code owner November 9, 2023 18:09

turbo-orchestrator bot added created-by: turbopack owned-by: turbopack labels Nov 9, 2023

jridgewell reviewed Nov 9, 2023

View reviewed changes

kwonoj force-pushed the url-full-resolve-context branch from ca15108 to f19102d Compare November 13, 2023 17:12

kwonoj requested a review from a team as a code owner November 13, 2023 17:12

kwonoj requested review from tknickman and arlyon November 13, 2023 17:12

turbo-orchestrator bot added the owned-by: turborepo label Nov 13, 2023

sokra reviewed Nov 13, 2023

View reviewed changes

crates/turbopack-ecmascript/src/references/esm/url.rs Outdated Show resolved Hide resolved

sokra reviewed Nov 13, 2023

View reviewed changes

rustc-ice-2023-11-13T17:05:44.397158Z-94125.txt Outdated Show resolved Hide resolved

kwonoj force-pushed the url-full-resolve-context branch from f19102d to 48bd511 Compare November 13, 2023 21:48

turbo-orchestrator bot removed the owned-by: turborepo label Nov 13, 2023

sokra requested changes Nov 13, 2023

View reviewed changes

kwonoj force-pushed the url-full-resolve-context branch 2 times, most recently from 63a1305 to 8010a70 Compare November 14, 2023 17:02

sokra approved these changes Nov 14, 2023

View reviewed changes

kwonoj force-pushed the url-full-resolve-context branch from 8010a70 to 13e7f62 Compare November 14, 2023 17:18

kwonoj force-pushed the url-full-resolve-context branch from 13e7f62 to 9261cb7 Compare November 14, 2023 23:42

arlyon approved these changes Nov 15, 2023

View reviewed changes

kwonoj force-pushed the url-full-resolve-context branch 3 times, most recently from a100e8b to 2720b60 Compare November 17, 2023 00:59

vercel bot had a problem deploying to Preview – turbo-site November 17, 2023 00:59 Failure

kwonoj force-pushed the url-full-resolve-context branch from 2720b60 to b98c5a8 Compare November 17, 2023 01:13

vercel bot had a problem deploying to Preview – turbo-site November 17, 2023 01:13 Failure

kwonoj added 2 commits November 16, 2023 17:14

fix(turbopack-ecmascript): support urlrewritebehavior::full

9791ff4

test(snapshot): update snapshot

8152044

kwonoj force-pushed the url-full-resolve-context branch from b98c5a8 to 8152044 Compare November 17, 2023 01:15

kwonoj merged commit e26a307 into main Nov 17, 2023
46 of 49 checks passed

kwonoj deleted the url-full-resolve-context branch November 17, 2023 01:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(turbopack-ecmascript): support urlrewritebehavior::full #6413

fix(turbopack-ecmascript): support urlrewritebehavior::full #6413

kwonoj commented Nov 9, 2023 •

edited by padmaia

Loading

vercel bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

jridgewell Nov 9, 2023

kwonoj Nov 9, 2023

github-actions bot commented Nov 13, 2023

sokra left a comment

github-actions bot commented Nov 13, 2023

kwonoj commented Nov 14, 2023

sokra commented Nov 14, 2023

kwonoj commented Nov 14, 2023

github-actions bot commented Nov 14, 2023

fix(turbopack-ecmascript): support urlrewritebehavior::full #6413

fix(turbopack-ecmascript): support urlrewritebehavior::full #6413

Conversation

kwonoj commented Nov 9, 2023 • edited by padmaia Loading

Description

vercel bot commented Nov 9, 2023 • edited Loading

github-actions bot commented Nov 9, 2023 • edited Loading

github-actions bot commented Nov 9, 2023 • edited Loading

🟢 CI successful 🟢

github-actions bot commented Nov 9, 2023

Linux Benchmark for 10232f6

github-actions bot commented Nov 9, 2023 • edited Loading

⚠️ Turbopack Benchmark CI failed ⚠️

jridgewell Nov 9, 2023

Choose a reason for hiding this comment

kwonoj Nov 9, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2023

Linux Benchmark for b30a55a

sokra left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 13, 2023

Linux Benchmark for 0410e52

kwonoj commented Nov 14, 2023

sokra commented Nov 14, 2023

kwonoj commented Nov 14, 2023

github-actions bot commented Nov 14, 2023

Linux Benchmark for 9c4b9bc

kwonoj commented Nov 9, 2023 •

edited by padmaia

Loading

vercel bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

Linux Benchmark for `10232f6`

github-actions bot commented Nov 9, 2023 •

edited

Loading

Linux Benchmark for `b30a55a`

Linux Benchmark for `9c4b9bc`