Call to updateConfig is ignored in astro:build:setup hook

[felixhuttmann]

Astro Info

Astro                    v4.16.9
Node                     v18.20.3
System                   Linux (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             client-server-targets-integration

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When writing an integration using the astro:build:setup hook, the call to updateConfig has no effect. For example, the below code, does not result in a build error:

        'astro:build:setup'({ target, updateConfig }) {
          updateConfig({
            vite: {
              esbuild: {
                target: 'invalid target',
              },
            },
          });
        },

In contrast, modifying the config in the astro:config:setup hook works correctly, so that the following code will result in a build error:

        'astro:config:setup'({ target, updateConfig }) {
          updateConfig({
            vite: {
              esbuild: {
                target: 'invalid target',
              },
            },
          });
        },

The documentation https://docs.astro.build/en/reference/integrations-reference/#astrobuildsetup states that the vite config should be modifiable also in the astro:build:setup hook. However, using the astro:config:setup hook is not an option for us, because the target: 'client' | 'server' parameter is available only in the astro:build:setup hook.

We need this feature because we want to set the esbuild target to safari12 only for the client, but not for the server. This is necessary because we need old browser compatibility on the client (thus "safari12), while on the server, the astrojs/node adapter started to use top-level await recently (https://github.com/withastro/adapters/blob/b0b247e5cd2fce02136e1998b26e893f02326fff/packages/node/src/server.ts#L12) which esbuild cannot transform. Before astrojs/node started to require top-level await, it was possible to use the astro:config:setup hook to transform all code to the safari12 target, which (together with some polyfills) allows us to run astro productively while still supporting safari 12. Because we need old browser compat, we are currently stuck on @astrojs/node 8.2.5 and astro 4.8.3.

A previous discussion on discord is at https://discord.com/channels/830184174198718474/872579324446928896/1303043016717762570.

What's the expected result?

updateConfig call is not ignored.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-7wdm2j?file=astro.config.mjs

Participation

• I am willing to submit a pull request for this issue.

[ematipico]

I believe the documentation isn't correct. In fact, updateConfig is only provided in astro:config:setup.

[felixhuttmann]

I believe the documentation isn't correct. In fact, updateConfig is only provided in astro:config:setup.

In this case, the type signature

            'astro:build:setup': (options: {
                vite: vite.InlineConfig;
                pages: Map<string, PageBuildData>;
                target: 'client' | 'server';
                updateConfig: (newConfig: vite.InlineConfig) => void;
                logger: AstroIntegrationLogger;
            }) => void | Promise<void>;

should be adjusted as well. Currently, the function updateConfig is callable but has no effect.

[philipschm1tt]

Via @Fryuni in discord

As it turns out, I fixed that one already on an ongoing PR for an RFC 😅
The config merge for the astro:build:setup is incorrect, it is using the function for merging the Astro config,
not the one for the Vite config.

But both Chris' example and your sample from the issue are incorrect tho.
The updateConfig takes changes to the Vite config directly.
TypeScript should have failed or at least warned about it.
It shouldn't have the vite: layer it should be:
updateConfig({
esbuild: { ... }
});

[SnithfferxDS]

I have this error with the newst version of both packages, astrojs 4.16.10 and @astrobot/db 0.14.3; but with the astrojs 4.16.9 works

Here a pick:

[vite] Error when evaluating SSR module \astro.config.mjs: failed to import "@astrojs/db"
|- Error: The parameter is incorrect.
\\?\\node_modules\@libsql\win32-x64-msvc\index.node
    at Object..node (node:internal/modules/cjs/loader:1715:18)
    at Module.load (node:internal/modules/cjs/loader:1318:32)
    at Function._load (node:internal/modules/cjs/loader:1128:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:218:24)
    at Module.require (node:internal/modules/cjs/loader:1340:12)
    at require (node:internal/modules/helpers:141:16)
    at requireNative (\node_modules\libsql\index.js:22:10)
    at Object.<anonymous> (\node_modules\libsql\index.js:45:5)
    at Module._compile (node:internal/modules/cjs/loader:1546:14)

[astro] Unable to load your Astro config

[rbsummers]

I believe the documentation isn't correct. In fact, updateConfig is only provided in astro:config:setup.

As @felixhuttmann mentioned, the astro:build:setup hook does also implement the updateConfig as reflected by the TS signature, so my proposal here is to change the documentation to properly expose it because this enables integrations to selectively build code for server OR client, which in turn this makes an integration to support legacy browsers possible.

The configuration passed to the updateConfig being ignored is actually a small issue within the code base and I intend to open a PR to get it fixed in a few days when I get some free time from work.