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

[Turbopack] Introduce OperationVc that wraps operations #70242

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

sokra
Copy link
Member

@sokra sokra commented Sep 19, 2024

What?

Introduce OperationVc that wraps operations

(Note: This was called VcOperation, but I changed it to OperationVc to better match the ResolvedVc naming convention -- @bgw)

We should only expose OperationVc into JS to connect to the whole computation (and be strongly consistent with the whole computation).

also fix query string Fixed in #70461

Why?

We want operations to be strongly consistent to the whole operation. Also HMR subscriptions should include the whole entrypoints and endpoint operation. The OperationVc type makes it easy to track that and enforces connecting to the operation correctly.

In regards of the ResolvedVc work, we also want operations to be explicit.

Closes PACK-3628

@ijjk ijjk added created-by: Turbopack team PRs by the Turbopack team. Turbopack Related to Turbopack with Next.js. labels Sep 19, 2024
Copy link
Member Author

sokra commented Sep 19, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@sokra sokra marked this pull request as ready for review September 19, 2024 07:50
@sokra sokra changed the title Introduce VcOperation that wraps operations [Turbopack] Introduce VcOperation that wraps operations Sep 19, 2024
@sokra sokra requested a review from bgw September 19, 2024 07:54
@ijjk
Copy link
Member

ijjk commented Sep 19, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Sep 19, 2024

Stats from current PR

Default Build
General
vercel/next.js canary vercel/next.js sokra/vc-operation Change
buildDuration 21.1s 19.2s N/A
buildDurationCached 18.4s 16.5s N/A
nodeModulesSize 409 MB 409 MB N/A
nextStartRea..uration (ms) 483ms 489ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js sokra/vc-operation Change
1187-HASH.js gzip 50.2 kB 50.2 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.3 kB 5.3 kB N/A
bccd1874-HASH.js gzip 53 kB 53 kB N/A
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 233 B 236 B N/A
main-HASH.js gzip 33.8 kB 33.7 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 0 B 0 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js sokra/vc-operation Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js sokra/vc-operation Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.44 kB 4.43 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary vercel/next.js sokra/vc-operation Change
_buildManifest.js gzip 747 B 745 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js sokra/vc-operation Change
index.html gzip 524 B 524 B
link.html gzip 539 B 539 B
withRouter.html gzip 520 B 521 B N/A
Overall change 1.06 kB 1.06 kB
Edge SSR bundle Size
vercel/next.js canary vercel/next.js sokra/vc-operation Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 203 kB 203 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js sokra/vc-operation Change
middleware-b..fest.js gzip 670 B 669 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31 kB 31 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js sokra/vc-operation Change
523-experime...dev.js gzip 322 B 322 B
523.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 322 kB 322 kB
app-page-exp..prod.js gzip 127 kB 127 kB
app-page-tur..prod.js gzip 140 kB 140 kB
app-page-tur..prod.js gzip 135 kB 135 kB
app-page.run...dev.js gzip 312 kB 312 kB
app-page.run..prod.js gzip 122 kB 122 kB
app-route-ex...dev.js gzip 37.1 kB 37.1 kB
app-route-ex..prod.js gzip 25.1 kB 25.1 kB
app-route-tu..prod.js gzip 25.1 kB 25.1 kB
app-route-tu..prod.js gzip 24.9 kB 24.9 kB
app-route.ru...dev.js gzip 38.7 kB 38.7 kB
app-route.ru..prod.js gzip 24.9 kB 24.9 kB
pages-api-tu..prod.js gzip 9.56 kB 9.56 kB
pages-api.ru...dev.js gzip 11.4 kB 11.4 kB
pages-api.ru..prod.js gzip 9.56 kB 9.56 kB
pages-turbo...prod.js gzip 21.3 kB 21.3 kB
pages.runtim...dev.js gzip 27 kB 27 kB
pages.runtim..prod.js gzip 21.3 kB 21.3 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 2.35 MB 2.35 MB
build cache
vercel/next.js canary vercel/next.js sokra/vc-operation Change
0.pack gzip 2.04 MB 2.03 MB N/A
index.pack gzip 146 kB 145 kB N/A
Overall change 0 B 0 B
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 8e51f56

@ijjk ijjk added the tests label Sep 19, 2024
@sokra sokra force-pushed the sokra/vc-operation branch from 3474ab1 to 644f5a9 Compare September 19, 2024 16:05
@mischnic
Copy link
Contributor

Some docs would be nice. I wouldn't know when to use VcOperation instead of Vc and what .connect() actually does

bgw added a commit that referenced this pull request Sep 24, 2024

Verified

This commit was signed with the committer’s verified signature.
unstubbable Hendrik Liebau
#70421)

Noticed these files while reviewing #70242

Mark them as generated so that github & graphite collapse them when reviewing code.

```
$ git ls-files | git check-attr -a --stdin | grep __snapshots__
```

```
.config/ast-grep/rule-tests/__snapshots__/no-context-snapshot.yml: linguist-generated: true
examples/with-jest-babel/__tests__/__snapshots__/snapshot.tsx.snap: linguist-generated: true
examples/with-jest/__tests__/__snapshots__/snapshot.tsx.snap: linguist-generated: true
examples/with-typescript-graphql/test/__snapshots__/index.test.tsx.snap: linguist-generated: true
test/development/acceptance-app/__snapshots__/ReactRefreshLogBox.test.ts.snap: linguist-generated: true
test/development/acceptance/__snapshots__/ReactRefreshLogBox.test.ts.snap: linguist-generated: true
test/development/acceptance/__snapshots__/ReactRefreshLogBoxMisc.test.ts.snap: linguist-generated: true
test/development/acceptance/__snapshots__/error-recovery.test.ts.snap: linguist-generated: true
test/development/basic/__snapshots__/next-rs-api.test.ts.snap: linguist-generated: true
test/production/eslint/test/__snapshots__/next-build-and-lint.test.ts.snap: linguist-generated: true
```
bgw
bgw previously approved these changes Sep 24, 2024
Copy link
Member

@bgw bgw left a comment

Choose a reason for hiding this comment

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

If we're going to allow #[turbo_tasks::value]s to store VcOperation, we're going to need to make it impl ResolvedValue.

However, that doesn't really seem right because a VcOperation is not truly "resolved".

What I care about for local tasks is just that this is non-local, so the questions are:

  • Should we rename ResolvedValue? What should we rename it to? NonLocalValue?
  • Is there a reason we might want ResolvedValue in the future? If so, we can keep it around and just make NonLocalValue a supertrait of ResolvedValue?

Copy link
Member

bgw commented Sep 24, 2024

Some docs would be nice. I wouldn't know when to use VcOperation instead of Vc and what .connect() actually does

Yeah, I agree. It's pretty confusing to me too.

@bgw bgw force-pushed the sokra/vc-operation branch from 644f5a9 to 98361eb Compare November 8, 2024 19:03
@bgw
Copy link
Member

bgw commented Nov 8, 2024

I've rebased this PR, but we still need to solve:

  • A way to mark functions as operations, making them return VcOperation directly, avoiding the runtime checks that make sure a function isn't local.
  • Documentation of where and when to use VcOperation.
  • Documentation of what .connect() does, and when to use it.

@bgw bgw dismissed their stale review November 8, 2024 22:01

I'm owning the work to rebase this and get it ready to merge.

@bgw bgw force-pushed the sokra/vc-operation branch 2 times, most recently from 988e6fa to a7973b1 Compare November 13, 2024 18:59
@bgw bgw changed the title [Turbopack] Introduce VcOperation that wraps operations [Turbopack] Introduce OperationVc that wraps operations Nov 13, 2024
@bgw bgw force-pushed the sokra/vc-operation branch from a7973b1 to 93893d6 Compare November 16, 2024 01:36
@bgw bgw force-pushed the sokra/vc-operation branch from 93893d6 to 9166c5a Compare November 25, 2024 22:31
@bgw bgw force-pushed the sokra/vc-operation branch from ccc3bfb to 494ba77 Compare December 9, 2024 18:57

Verified

This commit was signed with the committer’s verified signature.
unstubbable Hendrik Liebau
only expose OperationVc into JS to connect to the whole computation (and be strongly consistent with the whole computation).
@bgw bgw force-pushed the sokra/vc-operation branch from 494ba77 to 8e51f56 Compare December 9, 2024 21:43
@bgw bgw merged commit 3c414b9 into canary Dec 9, 2024
116 of 121 checks passed
Copy link
Member

bgw commented Dec 9, 2024

Merge activity

  • Dec 9, 5:48 PM EST: A user merged this pull request with Graphite.

@bgw bgw deleted the sokra/vc-operation branch December 9, 2024 22:48
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Turbopack team PRs by the Turbopack team. locked tests Turbopack Related to Turbopack with Next.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants