next.js
d240d450 - refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types (#72871)

Commit
1 year ago
refactor(turbo-tasks): Remove public OperationVc constructor, introduce a macro annotation for creating OperationVc types (#72871) Introduces a new `operation` argument to the `#[turbo_tasks::function]` macro for creating functions that return `OperationVc`: ``` // this function's external signature returns an `OperationVc` #[turbo_tasks::function(operation)] async fn multiply(value: OperationVc<i32>, coefficient: i32) -> Result<Vc<i32>> { Ok(Vc::cell(*value.connect().await? * coefficient)) } ``` This is important to solve a few major footguns with manually-constructed `OperationVc`s: - It can be hard to know if a `Vc` is an operation or not, and the only warning you'd get if you got it wrong was a runtime error. - Local-task-based resolution (#69126) will implicitly create local `Vc`s if an argument isn't resolved. - ~We want to enforce that all arguments to `OperationVc`-returning functions are also `OperationVc`s or non-`Vc` values. Otherwise you could end up with a stale/wrong `OperationVc`.~ Scrapped, this was too hard/impractical, see below. This removes the public constructor. Methods are not currently supported because: 1. Operations are uncommon, and it's not worth the effort, IMO. 2. There's no way to make it work for dynamic-dispatched method calls, as we cannot resolve the type of `self`. It could be made to work for inherent impls and non-object trait types. --- This is basically implementing @sokra's TODO comment in the old `OperationVc::new` constructor: ``` // TODO to avoid this runtime check, we should mark functions with `(operation)` and return // a OperationVc directly ``` But with assertions for the function arguments, based on some discussion in DMs: <img src="https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/13952614-f421-4c8a-99b8-3ce5f19715ae.png" width="600"/> We allow *either* `ResolvedVc` or `OperationVc` as arguments because: - Only accepting `OperationVc` arguments was impossible to refactor to, as this made it "viral" and there are too many places where we need to use operations that have too many dependencies. - While it makes sense for `State` to require `OperationVc` as arguments to any operation (keeps the whole dependency/call tree alive), for collectibles, sometimes you want the arguments to be `ResolvedVc`. It's just a matter of if you want to include collectibles generated when creating the arguments or not.
Author
bgw bgw
Parents
Loading