fix: merge config doesn't work when user configs as a function.

[valcosmos]

Description

Additional context

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[bluwy]

The types already forbid passing a function as an argument, so I don't think we need this?

[valcosmos]

The types already forbid passing a function as an argument, so I don't think we need this?

Hi, thank u for your response. The reason I made this change was because I was having some issues.

I try to split vite.config.ts into vite.config.base.ts and vite.config.dev.ts and merge the two via mergeConfig

code here

When the defineConfig in vite.config.ts is passed in an object, everything is normal, but when it is passed in a function, the configuration in vite.config.base.ts seems to be invalid.

I try to look at the source code of defineConfig and mergeConfig, I think the problem is in mergeConfig, because defineConfig can accept an object and a function, and export it directly. After I made this modification, it seemed to fix the issue.

Is there a better solution for this?😭

[bluwy]

You can call the function before passing it to mergeConfig. I don't think this needs to be changed in Vite.

[valcosmos]

You can call the function before passing it to mergeConfig. I don't think this needs to be changed in Vite.

OK, thank u for your advices, much appreciate. 👍

[Minhir]

@bluwy

The types already forbid passing a function as an argument, so I don't think we need this?

Not really. mergeConfig now accepts records.

export function mergeConfig(
  defaults: Record<string, any>,
  overrides: Record<string, any>,
  isRoot = true,
): Record<string, any> {
  return mergeConfigRecursively(defaults, overrides, isRoot ? '' : '.')
}

But from TS point of view function is assignable to
records microsoft/TypeScript#53484

So code below is valid but not working

mergeConfig(
  defineConfig(() => ({})),
  {},
);

I'm agreed that it is better to avoid calling functions automatically because in that case we can have unobvious behaviour with initialization order. But we need to forbid passing functions in mergeConfig types.

[bluwy]

That's a strange TS behaviour, I'm not sure how we can forbid passing functions strictly though, the Record<string, any> doesn't represent the root Vite config only, it could represent a sub-config and there isn't a nice way to type it.

[Minhir]

@bluwy Record<string, unknown> will do the trick. I can prepare a PR.

export function mergeConfig<T>(
  defaults: Record<string, unknown>,
  overrides: Record<string, unknown>,
  isRoot = true,
): Record<string, unknown> {
  return {};
}

mergeConfig(
  {base: './', root: 'src', build: {outDir: 'dist'}},
  // ❌ Argument of type '() => {}' is not assignable to parameter of type 'Record<string, unknown>'.
  () => ({}),
);

[bluwy]

Interesting. I think we'd still need the return type to be Record<string, any> to not break the ecosystem, but for the inputs that could be fine.