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

Astro 4.5 creates severe problems and side effects for environmentals, potentially other configs - with resolution #10569

Closed
1 task
narration-sd opened this issue Mar 26, 2024 · 15 comments
Labels
needs response Issue needs response from OP

Comments

@narration-sd
Copy link

narration-sd commented Mar 26, 2024

Astro Info

Astro                    v4.5.9
Node                     v20.11.1
System                   Windows (x64)
Package Manager          unknown
Output                   hybrid
Adapter                  @astrojs/netlify
Integrations             @astrojs/react
                         @sanity/astro

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

any browser

Describe the Bug

I'm going to go step by step here, but you are welcome to skip to the Realistic Solution, not far below :)

How the problems show...

  1. the problem presents initially as losing all import.meta.env.PUBLIC_SANITY_NAME environmentals, on astro 4.5.x
  2. the solution proposed on first report is, 'just use loadEnv instead to get your environmentals''
  3. using loadEnv begins a flood of the most incorrect and misleading errors I have ever seen, especially from any deploy to Netlify and also Vercel -- which have operated flawlessly on this complex app for months.
  4. The actual problem, in each case, boils down to one fact: that importing loadEnv in a file to gain variables, or importing a variable from such a file, entirely confuses Vite, so that it attempts to load browser client code (Astro islands) as if it were server code, whenever deployed in a server situation. This results in the very uninformative and fatal errors.
  5. The cascade of false errors stems from this, and the worst show the same on Vercel deploy as on Netlify, looking like a piece of boilerplate each is using, which has nothing at all to do with the actual case. There also can be an error from Vite itself, in some situations, which is also fully misleading, as it talks about 'client', but this is its own client, nothing to do with anything in the app sources or for example Sanity imports, I found in Vite code.

How can we resolve this?

I discovered two ways.

  1. In the end, to prove out several points and isolate when a week's high effort disclosed nothing useful on any intuition from the bogus errors, I created an Astro 4.5.9 from a fork, obfuscated its identity to avoid issues none of us would like, and made one change: I reverted the source of all problems, the line inserted into the 4.5 build from a worthy attempt to fix an obscure problem, whose actual conversation is here:, and published this, also obscurely.

    Using this single reversion, my complex app returned to working entirely properly, operating Sanity's very interprocess-entwined Presentation edit-from-web-page technology perfectly -- and using only the desirable import.meta.env.

  2. I had in fact finally made the situation operate with Astro's current own deployment.

    But what it took, after working through all the erroneous messaging and then obscure clue-hunting and easter-egging piece-by-piece into realizing the nature of the failure and causes, never visible itself...

    ...first of all, isolating all uses of environmental variables, some of which are intertwined and complicated, into two entire copies. One for the server side uses, employint that loadEnv which forces this, and the second copy employing import.meta.env, for all the client-side use equally requiring that, some of which are rather obscure, as they occur inside components non too simple, passed by props.

    It's important to note that none of the client-side variables could operate unless I opened loadEnv to import 'everything', thus allowing import.meta.env to see them. Thus all safety issues with not prefixing environmentals were opened occur. That's because the reverted code change prevents setting the envPrefix which properly governs this for import.meta.env.

    Finally, all my Vite config had to be moved inside of astro.config.ts, since vite.config.ts was ignored, making that far more complicated than I want it to be, for the developer/appbuilder-cultured persons this Astro-Sanity easy-onramps, full Visual Edit solution is intended to be for.

Understanding the Framing

Honestly, I can't think the 'just use loadEnv' advice is a solution whose results Astro would want to have occur, in any way. If you peruse some of the false error screens and quotes in prior versions of this issue, I think you will agree this is nothing a using person should be confronted with. Indeed, I found lonely references to some of them on the web -- plaintive and never with an answer, because who could give one?

Further, when I look at the actual issue the 4.5-included problem change attempts to solve, it is quite obscure, itself -- occurring in a Vite custom plugin, and really sounding like it needs to be fixed at its source. The repair attempt was by someone who admitted they hadn't fully understood. I had some side conversation where it was suggested this 'fix' was to cure general risks in Astro's use of Vite, but I don't see this in the problem.

A Realistic Solution

I would propose, then, respecting all possibilities, including the open-ended one -- but beginning,in practical mind, with full repair very much in the present:

  • first of all, revert the line ending use of vite.config.ts in a next most minor release of Astro 4.5 , now and forward.

    It cannot be worth the pain shown above, and the risks to Astro's future in opportunities, to leave it there with its consequences. Let Astro apps continue work as they have been, be configured as they have been.

  • then, create a healthy, accurately focused solution for the issue that drove this problem change. Focus it on exactly what occurred, likely by filtering in the Astro code which gets involved as shown with the original issue.

  • if indeed the original problem hinted into further issues, prevent those in the same fashion, with focus and accuracy.

    I'd feel this solution set would live up to the excellence which Astro's premise, and the continuously evidenced design and architectural creativities of its core team, have always shown. And it's what your public presentation and those guiding it would like to live and be so lively, isn't it so?

Thank you for listening.

What's the expected result?

Expected would be that complex, environmental variable-dependent applications would continue to work flawlessly, wherever developed or deployed, in Astro as it moves to 4.5.x

[n.b. the minimal example that follows only shows the initial problem, not the waterfall of difficulty that results as you try to solve it....]

Link to Minimal Reproducible Example

https://stackblitz.com/~/github.com/narration-sd/github-lerf2k-suvouc

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 Mar 26, 2024
@narration-sd narration-sd changed the title Astro 4.5 creates severe problems and side effects for environmentals, potentially others - with resolution Astro 4.5 creates severe problems and side effects for environmentals, potentially other configs - with resolution Mar 26, 2024
@narration-sd
Copy link
Author

narration-sd commented Mar 27, 2024

p.s. one thing I could add, useful if it isn't already clear.

A reason this is so challenging to troubleshoot, beyond the insensible messages, is that most of the problems are observable only on deployment.

The system must be built, in order to have the improper separation attempt between server and browser client code to occur and fail as it does. And though the hybrid-using system can be built on a laptop, the result can't be previewed there, due to the adapter.

Once again, am imagining this is something a customer would be happy not to run into...

@matthewp
Copy link
Contributor

matthewp commented Mar 27, 2024

This issue is, again, far too verbose. You don't need to explain things like you're writing a blog post. Just state the problem as concisely as possible. From my reading it is:

  • fromEnv causes errors
  1. First, what is fromEnv? I don't see that in your example. Do you mean loadEnv?
  2. What are the errors, specifically? From your example I see the variables being logged.

Please answer these two questions in as few words as possible. 1 or 2 sentences each should be enough.

@matthewp matthewp added needs response Issue needs response from OP and removed needs triage Issue needs to be triaged labels Mar 27, 2024
@narration-sd
Copy link
Author

Hi Matthew, and I appreciate you're in the midst of triage with new release, surely.

  • yes, I mean loadEnv. I'll fix that. Vite's own loadEnv.
  • I left the errors out because they're in the preceding linked issues, which this one coalesces now that I have solutions. They're screenshots, and do not interpret to the actual problem whatsoever -- that's what makes all this complicated. I'll put them in;, following.
  • On length, I carefully put a brief list at top. The explanation is what I'd want, as tight as I could make it, if I had been facing such a problem myself, over not a few years of experience. Again, I appreciate your situation entirely, be sure, and with a simile, also the appreciation that always goes with the Astro team.

@narration-sd
Copy link
Author

narration-sd commented Mar 27, 2024

Ok, at least I was consistent; it was late, and it's early now. loadEnv has become properly stated.

  • next come the error screens. The reason they aren't visible in the minimal example is because you need to be deployed on a server to have the cause activate, thus be able to see them, the only place they occur.

  • this is the screen you get on either Netlify or Vercel, from applying the advice to 'just use loadEnv'. Please be aware the problem or its actual symptoms have nothing whatever to do with what it says. Thus began the walk through deep woods.

Screenshot 2024-03-21 204515

  • you've already tried out the demo app, which puts on screen that the client variable isn't getting through, another but less false lead on the actual problem, so we'll skip that one from what was shown earlier.

  • although you can't demo the deployed crash locally (can't run the adapter in preview), the better clue can be there in some build failures, depending on what you're trying.

    The trick is to understand why. I finally was able to establish that in any fail case, even some pretty obscure ones, it's because Vite will try to link, as we used to call it, Node code into Astro's client-only islands .

    This false linkage attempt, and its necessary build fail or app crash, will happen any time you even so much as make an unused (or used) import of loadEnv into a client island's file,, or indeed import a variable that comes from such a loadEnv-importing file.

    Please read that last short paragraph twice, and we'll have arrived at the true understanding.

Screenshot 2024-03-22 163500

  • Ok. I had, I thought but not, another screenshot from where I had most solved, duplicating every environmental config, moving all my Vite config into astro.config, and being exceedingly careful where each kind of environmental access was used, which is the only way to get astro@4.5 working.

    But, it still didn't, and I got one of those brown screens as the first here, just with slightly less obscuring language once I realized so, though nothing that would tell you the problem.

    It turned out the cause was a sneak path -- I had passed some config of the loadEnv derivation into a complex, non-visible island component which runs the Sanity visual edit for screens -- and this in a fresh new way caused Vite to improperly link/bundle, with not a build fail, just Netlify 'function' crashing. And this is one of many cases which doesn't show in build, only in server crashes.

  • Fixing that, the app system fully recovered, and it felt really fun to exercise all the things it can do with Sanity. I don't think I have to tell you, Matthew, how many false trails explored, and intuitions required out of deep background knowledges, to have success in working out this kind of solution. If I've seemed to hit the loud pedal anywhere, it's because I don't want anyone else to face this.

    Somehow not feeling very alone there, and definitely said with that mutually respectful smile...

@narration-sd
Copy link
Author

@matthewp Matthew, reviewing, I realized the error screens above don't quite illustrate the central failure, so I've re-created the best view you can get of it, here.

  • as described, all you have to do for failure is import a value into an Astro island component, which has come from a file using loadEnv. In this case, we're actually importing one of it's retrieved environmentals, siteUrlPath, as you can see from source in the upper right pane, and using it minimally.
  • I made that pane small so you could see as much as possible of the resulting error listing. In fact, there are very many of those warnings that Vite has 'externalized for browser compatibility', all Node elements which should never have been involved for client code.
  • finally you see the build crash, where it's hit something it can't try to treat this way.
  • with less fortune, as mentioned there are many other ways client involvement of any kind with loadEnv-related imports can cause, not a build crash, but those entirely obscure runtime failures on the server, Vercel as well as Netlify.

Remedies are as described in the original post.

Screenshot 2024-03-27 105450

@matthewp
Copy link
Contributor

matthewp commented Mar 27, 2024

Again I ask that you be less verbose. I cannot help if I need to read so much to understand the problem.

Ok, so from what I can tell the problems are now:

  1. When you deploy to Netlify you get errors
  2. You are attempting to import the Astro config into your client component and can't get the exported values.

For (2), do not load your Astro config in a client component. That is not supported and is not a good thing to do, you are loading a lot of code in the client by doing so. Instead create a file like perhaps src/client-config.ts and put any client config there. You can use import.meta.env.


I think the crux of the issue is you want a single file where you read environment variables and you want that to be the config file. Let me state very clearly you cannot do that in Astro. You need to use both loadEnv for your config file and import.meta.env in any other code.

Please try this and let me know if (1) is still happening.

@narration-sd
Copy link
Author

narration-sd commented Mar 27, 2024

  • I only used astro.config in latest example as it was convenient. All real work has used entirely separate files for importing envs.
  • I already have the full system operative, as said, by separating all environmental use equivalently to what you've said.

Necessarily further:

  • contrary to your' 'you can't' statement, loadEnv has never been necessary before 4.5's little addition.
  • when you avoid allowing a vite.config.ts file, you disallow defining envPrefix, which import.meta.env requires to use names not starting with VITE_. Those are required for many real-life systems that Astro can be used with.
  • thus you have to call loadEnv with an empty string third argument, which defeats safety of env names, so that import.meta.env will work elsewhere.
  • you've also forced that any needed Vite config, which can get pretty elaborate but starts perhaps with aliases, to be put into astro.config.
  • this last is why envPrefix can't be set at event timing where it can make import.meta.env work as needed for other systems startup.

I'm apologetic, Matthew, if being polite and complete make things difficult, truly. I can understand you, however, I think.

@matthewp
Copy link
Contributor

If the issue here is that loadEnv doesn't fit your needs, you have many options:

  • process.env if they are real environment variables. There are no prefix restrictions using this.
  • A package like dotenv (there are many alternatives) that handle loading from a .env file.

Astro does not solve loading environment variables from different sources into your Astro config. This is not a feature of the framework, it's something you need to figure out depending on your needs, using ecosystem packages or built-in Node.js features.

@narration-sd
Copy link
Author

narration-sd commented Mar 27, 2024

Matthew, aren't you narrowing this so your 'works as re-designed' arguments fit?

I've handled everything you talk about, and certainly am aware of other methods -- once the ferocious server crashes could be understood. [also memory tempts me to think l loadEnv would still have to be in there to cover the envPrefix problem in all of this]

The point I see is that you guys are very unnecessarily breaking things in a way that will cause this for others. It may be uncomfortable to have that pointed out, for which I've been respectful and sorry.

Would you yourself accept such crashes, without fully understanding them and seeing remedies that were not full of risk? 'Just' using two separate systems, where one can cause such upsets by any inadvertent presence, not even use, of the 'wrong' one., seems to me full of risk.

I've addressed whatever prompted the change actually in the suggestions. In a long career as a 'real' consultant, those are the kinds of solutions that have been praised, to 'really work'.

@narration-sd
Copy link
Author

narration-sd commented Mar 27, 2024

I guess I could also remind, that it is not imagined 'others' that I see this problem affecting.

Patient as I am in it, Astro persons' response so far have caused me to seriously consider -- against -- putting forward the system for Sanity I have months of successful effort into, where I know very well how tense and brittle much of the user base is about anything past what they can glean from copying scaffolds without challenges. And especially, mystery errors.

Perhaps I can hide enough and comment enough what needs to go on to avoid their confrontation if you don't open up and fix this, but when they try to extend, as you know such persons also are prone to, for 'features', what then?

I'm not and never have been a 'business person', nor other things you may imagine, but I am pretty experienced in what they feel to need, and do appreciate in broadly creative responses. Sanity and Astro could help the future of every one involved, it has seemed. If it can be made approachable and solid.

I suspect you'd be interested in some of the approaches I've invented to manage that for them. If we can get there, Matthew...it's been tough enough already, sometimes kind of satisfying....

Out for some exercise, fresh air now, being quite human, and much more of years than you probably think, also :)

Clive

@ematipico
Copy link
Member

The point I see is that you guys are very unnecessarily breaking things in a way that will cause this for others. It may be uncomfortable to have that pointed out, for which I've been respectful and sorry.

Sorry if this happened to you. It happened to other users, and we always try to fix regressions as fast possible, and we always try to suggest workarounds in the meantime. It's unfair to judge our work like this.

There's also the fact sometimes users are unintentionally leveraging issues, and once we fix the issue, things don't work. It happens, it happens and it will happen. Please keep this in mind.

Would you yourself accept such crashes, without fully understanding them and seeing remedies that were not full of risk?

No, although if we can't understand, that doesn't mean we don't accept it. We can't understand your issue. We're asking you to:

  • provide a minimal reproduction
  • tell us how to replicate the issue
  • try to be succinct describing the issue

We will triage it once we have clear steps.

The more succinct you are, the faster your issue gets solved.

@narration-sd
Copy link
Author

@ematipico, thanks, I appreciate what you say, especially your approach also, and caught it before out the door.

I'll think what I can do. With a problem so obscurant of itself, it's hard indeed to reduce what it takes to understand 'steps' to play with it -- I remind that much of it only occurs on deploys. Have you a system where you can actually see inside what happens there? Things like netlify serve appear utterly incapable for this -- I tried it.

But to see Vite fail by trying to load client code as if it were server code, which is the primary actual failing, this I might be able to cook up a suitable way to demo. I don't know that it can run on StackBlitz, but if so, I could publish a repo.

Thanks for now, and on your timezone especially :)
Clive

@matthewp
Copy link
Contributor

@ematipico I understand the steps now. It's the same thing as in previous issues.

@narration-sd I understand your point of view. However, this is working as designed. Astro does not have an abstraction for .env files in the Astro config, and has never had one intentionally. What was previously working for you was a temporary accident. And we cannot restore every temporary accident just because some people relied on it.

As Astro does not attempt to solve the problem you are hoping to solve I would advise you participate in this roadmap issue which might solve the problem you're experiencing.

In the meantime I would advise this: do not try to abstract all of your environment variables into a single file and use it in your config, your app, and in the client. Accept the fact that you're going to have some redundant code, as it's necessary at this time.

@narration-sd
Copy link
Author

Thanks also, Matthew, and fully understood I promise you.

It's not fundamentally the redundancy, little as users here will like that; it's the risk.

I've hit this block on 'we never intended to handle enviromentals otherwise' from others, but it seems to go deeper. That you've blocked off the vite.config.ts is what I really heard was center of intention now, according to this design rule, and that's where problems begin and multiply from. I remind that as far as I've seen, import.meta.env can't work with needed ENV_NAMES, unless loadEnv is used earlier, because Vite config is prevented from running early now.

Result: integrations can't be configured. It's a loop all this runs in, and then you have the risks of blind server errors -- by any mistake in where client imports come from.

I appreciate this is formidable, and that's why I've suggested another path is needed for what that problem one-liner in 4.5 does.

There's probably more behind the intensity to employ it, however it was generated, I picked up from some early nice chat as it always is with Blu. But do you need this blanket block truly?

Perhaps I could get into futures discussions, but the problem is now, thus asking to unwind it, as recent long experience hasn't shown whatever dangers there may be to show themselves -- and this change has done.

Now, that walk :) , and truly thanks, to both of you.

@matthewp
Copy link
Contributor

Ok, thanks for taking the time to have the conversation. Since we aren't going to restore loading the Vite config, I'm going to close this issue. I encourage you to participate in the astro:env RFC and hopefully that can help.

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