Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In 4.16.14: middleware suddenly reads config.ts with astro build, it didn't since at least 4.10 #12527

Open
1 task
corneliusroemer opened this issue Nov 25, 2024 · 7 comments
Labels
needs response Issue needs response from OP

Comments

@corneliusroemer
Copy link

corneliusroemer commented Nov 25, 2024

Astro Info

Astro                    v4.16.14
Node                     v22.11.0
System                   macOS (arm64)
Package Manager          npm
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/tailwind
                         @astrojs/react
                         @astrojs/mdx

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

No response

Describe the Bug

When upgrading from 4.16.14 (via dependabot), astro build suddenly fails, it worked fine in 4.16.13.

We have a JSON config file that is read by config.ts at runtime. It's not present at build time. This was never an issue until 4.16.14 where we suddenly get build failures if the file is not present (no build failure if the file is present).

Repro:

  1. Check out loculus-project/loculus@7534e92
  2. cd website && npm ci && npm run build
  3. Observe result

This previous commit with only difference being 4.16.13 builds fine: loculus-project/loculus@94732ca

Stacktrace:

23:33:17 [vite] ✓ built in 5.38s
ENOENT: no such file or directory, open 'tests/config/website_config2.json'
  Location:
    /Users/corneliusromer/code/loculus/website/node_modules/astro/dist/core/build/pipeline.js:105:57
  Stack trace:
    at Object.readFileSync (node:fs:441:20)
    at getWebsiteConfig (file:///Users/corneliusromer/code/loculus/website/dist/server/chunks/config_DyPyM6lX.mjs:407:15)
    at ModuleJob.run (node:internal/modules/esm/module_job:268:25)
    at async BuildPipeline.retrieveManifest (file:///Users/corneliusromer/code/loculus/website/node_modules/astro/dist/core/build/pipeline.js:105:57)
    at async staticBuild (file:///Users/corneliusromer/code/loculus/website/node_modules/astro/dist/core/build/static-build.js:109:5)
    at async AstroBuilder.run (file:///Users/corneliusromer/code/loculus/website/node_modules/astro/dist/core/build/index.js:182:7)
    at async build (file:///Users/corneliusromer/code/loculus/website/node_modules/astro/dist/cli/build/index.js:24:3)
    at async cli (file:///Users/corneliusromer/code/loculus/website/node_modules/astro/dist/cli/index.js:175:5)

What's the expected result?

Build works without issue as it did until 4.16.13

Link to Minimal Reproducible Example

loculus-project/loculus@7534e92

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 25, 2024
@corneliusroemer corneliusroemer changed the title Chang of behaviour in 4.16.14, file that wasn't read before suddenly required, astro build fails with ENOEND in pipeline.js:105:57 Regression in 4.16.14: file that wasn't read before suddenly required, astro build fails with ENOENT in pipeline.js:105:57 Nov 25, 2024
@ematipico
Copy link
Member

ematipico commented Nov 25, 2024

That file config.ts is read in the middleware, and the middleware is also executed during the build for prerendered pages. It's documented in documentation.

It's been like this since the very beginning. Can you please narrow down the issue and provide a reproduction?

@ematipico ematipico added needs response Issue needs response from OP needs repro Issue needs a reproduction labels Nov 25, 2024
Copy link
Contributor

Hello @corneliusroemer. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Nov 25, 2024
@corneliusroemer
Copy link
Author

corneliusroemer commented Nov 25, 2024

I agree that the reproduction isn't minimal but it is a precise reproduction occurring between to patch versions. You can clone it in 30s, npm install and there you go. Minimizing a TS app isn't easy.

If the reason is that "the middleware is also executed during the build for prerendered pages. It's documented in documentation." this must mean that in a patch version, some new pages prerendered that didn't before. Is that possible?

@corneliusroemer
Copy link
Author

corneliusroemer commented Nov 26, 2024

Ok, it does seem to happen in the middleware and we had a code change there 6 days ago. So it's possible this should have always been an error and it just wasn't in the particular Astro version we were using at the time, likely 4.16.7

Does that sound like a plausible explanation?

It sounds similar to this issue? #12495

@corneliusroemer
Copy link
Author

corneliusroemer commented Nov 26, 2024

Ok here's a stackblitz: it passes with 4.16.13 and fails with 4.16.14 when you run npm run build: https://stackblitz.com/edit/github-patrsd-2vc61n?file=src%2Fconfig.ts

It's got only tiny modifications compared to the stackblitz from #12495: reading a file that doesn't exist in config.ts - this wasn't an issue till 4.16.14

Builds with 4.14, builds with 4.10, so if it the behavior now is how it should be it hasn't been correct for a long time.

@corneliusroemer corneliusroemer changed the title Regression in 4.16.14: file that wasn't read before suddenly required, astro build fails with ENOENT in pipeline.js:105:57 In 4.16.14: middleware suddenly reads config.ts with astro build, it didn't since at least 4.10 Nov 26, 2024
@ematipico
Copy link
Member

@corneliusroemer

The stackblitz you shared uses 4.16.13 anbd it fails. Also, why would you expect this to not fail? The example you shared shows that the code tries to read a file that doesn't exist, so it makes sense having the error.

In the latest patch, we fixed a minor bug around the middleware, so maybe, without knowing, you were leveraging the bug?

@ematipico ematipico removed the needs repro Issue needs a reproduction label Nov 26, 2024
@rktyt
Copy link

rktyt commented Nov 27, 2024

Currently, it seems that all code must be defined within onRequest.
However, it is also true that SSR has a lot of code that only needs to run once before onRequest.

In my case I have something like connecting with an another system through a socket file.
Honestly, I don't want to prepare something that is not necessary just to pass the build.
Even if the current behavior is correct, I don't want to accept it.
So I decided to apply the patch manually.

I don't know if it will be helpful, but I will list the patch that I have applied at hand.
I do not guarantee operation nor take any responsibility. (I have no idea how this part of the code is actually used)
Works fine in my project.
Note: I am using it in middleware mode, not standalone mode. Also, the server program is bundled with esbuild after astro build.

This is for [email protected].
patches/astro+4.16.15.patch (used by patch-package)

diff --git a/node_modules/astro/dist/core/build/pipeline.js b/node_modules/astro/dist/core/build/pipeline.js
index a960ded..1accc2f 100644
--- a/node_modules/astro/dist/core/build/pipeline.js
+++ b/node_modules/astro/dist/core/build/pipeline.js
@@ -12,7 +12,7 @@ import { createDefaultRoutes } from "../routing/default.js";
 import { findRouteToRewrite } from "../routing/rewrite.js";
 import { isServerLikeOutput } from "../util.js";
 import { getOutDirWithinCwd } from "./common.js";
-import { cssOrder, getPageData, mergeInlineCss } from "./internal.js";
+import { cssOrder, getPageData, mergeInlineCss, hasPrerenderedPages } from "./internal.js";
 import { ASTRO_PAGE_MODULE_ID, ASTRO_PAGE_RESOLVED_MODULE_ID } from "./plugins/plugin-pages.js";
 import { RESOLVED_SPLIT_MODULE_ID } from "./plugins/plugin-ssr.js";
 import { getPagesFromVirtualModulePageName, getVirtualModulePageName } from "./plugins/util.js";
@@ -102,7 +102,8 @@ class BuildPipeline extends Pipeline {
     }
     const renderersEntryUrl = new URL(`renderers.mjs?time=${Date.now()}`, baseDirectory);
     const renderers = await import(renderersEntryUrl.toString());
-    const middleware = internals.middlewareEntryPoint ? await import(internals.middlewareEntryPoint.toString()).then((mod) => {
+    // Probably it would be OK to import internals.middlewareEntryPoint only if there are prerendered pages.
+    const middleware = hasPrerenderedPages(internals) && internals.middlewareEntryPoint ? await import(internals.middlewareEntryPoint.toString()).then((mod) => {
       return function() {
         return { onRequest: mod.onRequest };
       };

Note: I created this patch because I have only recently started using astro and have not yet fully understood it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs response Issue needs response from OP
Projects
None yet
Development

No branches or pull requests

3 participants