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

felixhuttmann commented 1 week ago

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 commented 1 week ago

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

felixhuttmann commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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

withastro / astro