Skip to content

Commit 70dd18d

Browse files
authored
fix(migrate): move task cache settings in wrapped config callbacks (#2823)
The task cache migration from #2814 skipped `run.tasks` in configs written as a `defineConfig` callback with a type assertion, and printed no warning: ```ts export default defineConfig( (conf) => ({ run: { tasks: { build: { command: 'tsc && vp build', input: [...], output: [...] } } }, }) as UserConfig, ); ``` It looked through `as` directly around the config object, but not through the parentheses of the arrow body, nor through parentheses or assertions around the callback itself (`defineConfig(((conf) => ({ ... })) as T)`). `vp migrate` reported success, then `vp run` failed with `Failed to load task graph: Cache settings ... must be set under cache` and asked the user to run `vp migrate` again. The migration now looks through any mix of parentheses and type assertions around the config object and around its `defineConfig` callback. It also warns about tasks in a top-level `vite.config.*` object that is not the exported config, such as a variable the config refers to or an object passed to `mergeConfig`, instead of skipping them silently. Tasks created in other modules, for example by a shared helper, are still not detected; the migration rules page now says so. Found by the v1.0.0-rc.1 ecosystem smoke test (#2818): | Project | Before | After | | --- | --- | --- | | `vite-plus-ecosystem-ci/BlockNote` | 19 `vite.config.ts` files left unchanged without a warning; CI fails to load the task graph | All 19 migrate; a second pass finds nothing left to move | | `vite-plus-ecosystem-ci/cloudflare-os` | No warning | Warns about `typed-storage`'s `build` task, which is passed to a helper. Tasks built in shared helper modules still need manual changes | The new unit tests fail against the previous logic and pass with this change.
1 parent 5c08a72 commit 70dd18d

3 files changed

Lines changed: 77 additions & 21 deletions

File tree

‎crates/vp_migration/src/task_cache.rs‎

Lines changed: 74 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use vp_error::Error;
66

77
use crate::{
88
pack_config::can_edit_object,
9-
vite_config::{is_direct_recognized_config_object, pair_key_matches},
9+
vite_config::{is_define_config_call, is_direct_recognized_config_object, pair_key_matches},
1010
};
1111

1212
/// Task settings that Vite Task only accepts inside a task's `cache` object.
@@ -37,8 +37,10 @@ pub struct TaskCacheMigrationResult {
3737
/// Only static task objects in a direct config object are updated. Tasks with
3838
/// spreads, computed, escaped, or duplicate keys, comments the edit cannot
3939
/// place, or a `cache` value other than `true` or an object literal are
40-
/// reported in `manual_tasks` and left unchanged. Moved settings keep their
41-
/// text and indentation; the formatter nests them inside `cache`.
40+
/// reported in `manual_tasks` and left unchanged, as are tasks in a top-level
41+
/// object that is not the config itself, such as a variable the config refers
42+
/// to. Moved settings keep their text and indentation; the formatter nests
43+
/// them inside `cache`.
4244
pub fn migrate_task_cache_config(
4345
vite_config_path: &Path,
4446
) -> Result<TaskCacheMigrationResult, Error> {
@@ -51,12 +53,14 @@ fn migrate_task_cache_config_content(content: &str) -> TaskCacheMigrationResult
5153
let mut edits = Vec::new();
5254
let mut manual_tasks = Vec::new();
5355
for task in grep.root().dfs().filter(|node| node.kind() == "object") {
54-
let Some(name) = task_name(&task) else { continue };
56+
let Some((name, direct)) = task_name(&task) else { continue };
5557
let fields: Vec<_> = task.children().filter(is_cache_field).collect();
5658
if fields.is_empty() {
5759
continue;
5860
}
59-
match move_fields_under_cache(content, &task, &fields) {
61+
let task_edits =
62+
if direct { move_fields_under_cache(content, &task, &fields) } else { None };
63+
match task_edits {
6064
Some(task_edits) => edits.extend(task_edits),
6165
None => manual_tasks.push(name),
6266
}
@@ -77,27 +81,56 @@ fn migrate_task_cache_config_content(content: &str) -> TaskCacheMigrationResult
7781
TaskCacheMigrationResult { content: updated, updated: true, manual_tasks }
7882
}
7983

80-
/// Returns the task name when `object` is a task in `run.tasks` of a direct
81-
/// config object.
82-
fn task_name<D: Doc>(object: &Node<'_, D>) -> Option<String> {
84+
/// Returns the task name when `object` is a task in `run.tasks` of a top-level
85+
/// object, and whether that object is a direct config object. Objects nested
86+
/// in other objects, such as a plugin's `config()` result, are ignored.
87+
fn task_name<D: Doc>(object: &Node<'_, D>) -> Option<(String, bool)> {
8388
let task = value_pair(object)?;
8489
let tasks = task.parent().filter(|node| node.kind() == "object")?;
8590
let tasks_pair = value_pair(&tasks).filter(|pair| has_key(pair, "tasks"))?;
8691
let run = tasks_pair.parent().filter(|node| node.kind() == "object")?;
8792
let run_pair = value_pair(&run).filter(|pair| has_key(pair, "run"))?;
8893
let config = run_pair.parent().filter(|node| node.kind() == "object")?;
89-
// `is_direct_recognized_config_object` looks through `satisfies` but not `as`.
90-
let mut asserted = config.clone();
91-
while let Some(parent) = asserted.parent().filter(|node| node.kind() == "as_expression") {
92-
asserted = parent;
93-
}
94-
if !is_direct_recognized_config_object(&asserted)
95-
|| config.ancestors().any(|ancestor| ancestor.kind() == "object")
96-
{
94+
if config.ancestors().any(|ancestor| ancestor.kind() == "object") {
9795
return None;
9896
}
9997
let key = task.field("key")?;
100-
Some(key.text().trim_matches(['\'', '"']).to_owned())
98+
Some((key.text().trim_matches(['\'', '"']).to_owned(), is_direct_config(&config)))
99+
}
100+
101+
/// Whether `object` is a direct config object, looking through parentheses and
102+
/// type assertions around it and around a `defineConfig` callback, as in
103+
/// `defineConfig((env) => ({ ... }) as UserConfig)` and
104+
/// `defineConfig(((env) => ({ ... })) as UserConfigFn)`.
105+
fn is_direct_config<D: Doc>(object: &Node<'_, D>) -> bool {
106+
let outer = outermost_wrapper(object);
107+
if is_direct_recognized_config_object(&outer) {
108+
return true;
109+
}
110+
outer
111+
.parent()
112+
.filter(|arrow| {
113+
arrow.kind() == "arrow_function"
114+
&& arrow.field("body").is_some_and(|body| body.range() == outer.range())
115+
})
116+
.and_then(|arrow| outermost_wrapper(&arrow).parent())
117+
.filter(|arguments| arguments.kind() == "arguments")
118+
.and_then(|arguments| arguments.parent())
119+
.is_some_and(|call| is_define_config_call(&call))
120+
}
121+
122+
/// The outermost parenthesized or type-asserted expression around `node`.
123+
fn outermost_wrapper<'a, D: Doc>(node: &Node<'a, D>) -> Node<'a, D> {
124+
let mut outer = node.clone();
125+
while let Some(parent) = outer.parent().filter(|parent| {
126+
matches!(
127+
parent.kind().as_ref(),
128+
"parenthesized_expression" | "satisfies_expression" | "as_expression"
129+
)
130+
}) {
131+
outer = parent;
132+
}
133+
outer
101134
}
102135

103136
/// Returns the pair whose value is `node`, looking through parentheses and
@@ -595,9 +628,15 @@ mod tests {
595628
"export default defineConfig({ run: { tasks: ({ build: { command: 'x', env: ['A'] } }) } });",
596629
"export default defineConfig({ run: { tasks: { build: { command: 'x', env: ['A'] } } } } as UserConfig);",
597630
"export default { run: { tasks: { build: { command: 'x', env: ['A'] } } } } as UserConfig;",
631+
"export default defineConfig((env) => ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } }) as UserConfig);",
632+
"export default defineConfig(() => ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } } satisfies UserConfig));",
633+
"export default defineConfig(() => ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } }) satisfies UserConfig);",
634+
"export default defineConfig(((env: Env) => ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } })) as UserConfigFn);",
635+
"export default ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } });",
598636
] {
599-
let actual = migrate(input).content;
600-
assert!(actual.contains("cache: { env: ['A'] }"), "{actual}");
637+
let result = migrate(input);
638+
assert!(result.content.contains("cache: { env: ['A'] }"), "{}", result.content);
639+
assert!(result.manual_tasks.is_empty(), "{input}");
601640
}
602641
}
603642

@@ -608,7 +647,7 @@ mod tests {
608647
"export default defineConfig({ run: { cache: { tasks: true }, env: ['A'] } });",
609648
"export default defineConfig({ plugins: [{ config() { return { run: { tasks: { build: { env: ['A'] } } } }; } }] });",
610649
"export default defineConfig({ test: { run: { tasks: { build: { env: ['A'] } } } } });",
611-
"const config = { run: { tasks: { build: { command: 'x', env: ['A'] } } } }; export default config;",
650+
"const config = { run: { tasks: { build: { command: 'x', cache: { env: ['A'] } } } } }; export default config;",
612651
"export default defineConfig({ run: { tasks: { build: 'tsc', check: ['vp lint', 'vp build'] } } });",
613652
"export default defineConfig({ run: { tasks: { build: { command: 'x', cache: { env: ['A'] } } } } });",
614653
] {
@@ -618,6 +657,21 @@ mod tests {
618657
}
619658
}
620659

660+
#[test]
661+
fn reports_tasks_outside_the_config_object() {
662+
for input in [
663+
"const config = { run: { tasks: { build: { command: 'x', env: ['A'] } } } }; export default config;",
664+
"const shared: UserConfig = { run: { tasks: { build: { command: 'x', env: ['A'] } } } }; export default defineConfig(shared);",
665+
"export default mergeConfig(base, { run: { tasks: { build: { command: 'x', env: ['A'] } } } });",
666+
"export default withTests((() => ({ run: { tasks: { build: { command: 'x', env: ['A'] } } } })) as Fn);",
667+
"export function withTasks() { return { run: { tasks: { build: { command: 'x', env: ['A'] } } } }; }",
668+
] {
669+
let result = migrate(input);
670+
assert_eq!(result.content, input);
671+
assert_eq!(result.manual_tasks, ["build"], "{input}");
672+
}
673+
}
674+
621675
#[test]
622676
fn reports_tasks_that_need_manual_migration() {
623677
let input = r"export default defineConfig({

‎crates/vp_migration/src/vite_config.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -674,7 +674,7 @@ fn is_recognized_config_object<D: Doc>(object_node: &Node<'_, D>) -> bool {
674674
}
675675
}
676676

677-
fn is_define_config_call<D: Doc>(call_node: &Node<'_, D>) -> bool {
677+
pub(crate) fn is_define_config_call<D: Doc>(call_node: &Node<'_, D>) -> bool {
678678
call_node.kind() == "call_expression"
679679
&& call_node.field("function").is_some_and(|f| f.text() == "defineConfig")
680680
}

‎docs/guide/migrate-rules.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ The transform does not evaluate configuration code. It leaves a task unchanged a
9090
- a moved setting that already exists in `cache`;
9191
- a `cache` value other than `true` or an object literal.
9292

93+
It also warns about tasks in a `vite.config.*` object that is not the exported config itself, such as a variable the config refers to or an object passed to `mergeConfig`. Tasks created in other modules, for example by a shared helper function, are not detected. Move their settings by hand; `vp run` lists every task that still needs it.
94+
9395
With `cache: false`, the moved settings would have no effect, so decide whether to remove them or enable caching. On a project that is otherwise up to date, `vp migrate` prints these warnings without running the rest of the migration.
9496

9597
## Dependency Rules

0 commit comments

Comments
 (0)