search

withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
43.84k stars 2.28k forks source link

setUpEnvTs writes file even if no changes are needed #11303

Open NeoPhi opened 1 week ago

NeoPhi commented 1 week ago

Astro Info

Astro                    v4.10.3
Node                     v20.13.1
System                   macOS (x64)
Package Manager          unknown
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/react
                         @sentry/astro

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

No response

Describe the Bug

If src/env.d.ts is present but read-only by the user running a build, the build will fail. It seems setUpEnvTs will unconditionally rewrite this file even if there are no changes needed. This is problematic for CI/CD situations where the build user is not allowed to modify any files under the src/ directory.

https://github.com/withastro/astro/blob/dbbd0a22e249bb62d406aed016ed626a651a332d/packages/astro/src/vite-plugin-inject-env-ts/index.ts#L99

What's the expected result?

No files under src are modified if changes are not needed.

Link to Minimal Reproducible Example

N/A

Participation

Princesseuh commented 1 week ago

Do you have an example of a CI situation where src cannot be written to?

ematipico commented 1 week ago

@NeoPhi is your env.d.ts file committed in your repository?

@florian-lefebvre maybe we could add a check only during the build. If the file doesn't exist, we could throw a runtime error saying that the file should be committed. If it exists, we don't do any write operations. What do you think?

florian-lefebvre commented 1 week ago

That sounds reasonable to me, and easy to implement

bluwy commented 1 week ago

I suppose there's a risk breaking current setups where the src/env.d.ts is not committed, but I'm not sure how common is that. Maybe it would be simpler to not write the file if the file content is the same? Though I also agree with Erika that this is kinda a niche situation.

ematipico commented 1 week ago

True, definitely niche, but FS operations are always a big question mark and throw unexpected errors like in this case

matthewp commented 1 week ago

You should be able to create a new Astro project, run astro build having never run dev and it should work. It sounds like the suggestion here would break that. We don't have many required files in Astro and we shouldn't start adding them, imo. This is definitely a niche situation.

Could we add a --no-env or something? It's always been a little "controversial" to create files inside src as some people simply don't like such a thing. Could solve this.

NeoPhi commented 1 week ago

Our env.d.ts is committed since we added custom entries to support TypeScript as per https://docs.astro.build/en/guides/environment-variables/#intellisense-for-typescript

The --no-env suggestion seems like a nice solution, similar to yarn's --frozen-lockfile.