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

feat(runtime-config): export applyEnv and getEnv from nitropack #2404

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This adds applyEnv and getEnv as exports, as well as allowing them to operate on a custom env object instead of reading from process.env.

This would be a useful feature for frameworks to provide more accurate runtimeConfig at the build time. For example, we already inline these utils in nuxt/test-utils#655 and would need to do the same for nuxt/nuxt#24224 and nuxt/nuxt#26960.

Of course, that would be fine, but it would nicer to be able to simulate the runtime behaviour of Nitro with these utils, as suggested in nuxt/nuxt#24224 (comment).

I've included them in the core nitropack export but I would almost prefer nitropack/utils, perhaps, to allow space for other similar utilities?

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added the enhancement New feature or request label Apr 30, 2024
@danielroe danielroe requested a review from pi0 April 30, 2024 15:28
@danielroe danielroe self-assigned this Apr 30, 2024
@pi0
Copy link
Member

pi0 commented Apr 30, 2024

Thanks for PR ❀️

I think this needs to be more thorough thinking before exposing from nitro runtime utils. Diverging env definition one more level and allowing to complete override process.env (while for use cases like testing makes sense) can open the door to many more mistakes and implicit issues.

Therefore putting PR on draft state until have a chance to think more.

I've included them in the core nitropack export but I would almost prefer nitropack/utils, perhaps, to allow space for other similar utilities?

A shared (build time) util via nitropack/kit seems good idea.

@pi0 pi0 marked this pull request as draft April 30, 2024 18:30
@pi0
Copy link
Member

pi0 commented Apr 30, 2024

As another alternative approach we can think of different methods of runtime config expansion (via a util that is not env) so we don't add over complexity to "environment" definition but also allow a runtime method of customizing runtime config (this would help to somehow replicate "app config" extendability feature that is going to be deprecated in Nitro v3). I love to hear your thoughts about this method.

@danielroe danielroe changed the title feat(runtime-config): allow passing custom env object feat(runtime-config): export applyEnv and getEnv from nitropack Apr 30, 2024
@danielroe
Copy link
Member Author

It would be nice not to hard-code the use of process.env, even if we don't change Nitro runtime behaviour - it would help to avoid duplicating these utils (which would run the risk of diverging more quickly from Nitro)...

Are you thinking that we just rename these utils so they aren't environment-focused (ie. naming for clarity), so they can be used generically for expansion, either as build-time kit utils or within the runtime (first in env, and then later with new feature)? That sounds very doable - say the word and I'll refactor.

If you think this isn't so straightforward, I think we can go ahead and inline the existing nitropack behaviour in Nuxt for v3.12 like we did for nuxt/test-utils, and then take the time to get it right here, and switch downstream to using a forward-consistent implementation when it ships in nitropack/kit...

What do you think?

@pi0
Copy link
Member

pi0 commented Apr 30, 2024

It would be nice not to hard-code the use of process.env,

Usage of process.env is a (current) method of accessing standard "runtime environment variables" which varies per runtime and platform but is always accessible via process.env. What I want to avoid is to change the definition of system environment variables to possibly synthetic ones that can be injected from various sources and various stages of runtime that cause inconsistent behavior of utils but instead add a new method such as updateRuntimeConfig() for modifying runtime config in runtime without touching environment variables or the inference system currently possible by these utils.

Runtime config override order:

  • Currently: inlined > runtime env vars
  • Improved: inlined > runtime overrides > runtime env vars

If you think this isn't so straightforward, I think we can go ahead and inline the existing nitropack behavior in Nuxt for v3.12

Linked issues nuxt/nuxt#24224 and nuxt/nuxt#26960 are really good initiatives if the idea to have the useRuntimeConfig() in the builder namespace I think we should consider it for Nitro too at some point via kit. I imagine a builder/kit namespace useRuntimeConfig() to allow access and updateRuntimeConfig() provides these overrides merged with ones we inline to the runtime so the order would be:

inlined (kit ovverides + user config) > runtime overrides > runtime env vars

Not sure which one is your current priority for Nuxt but I think these are two separate features we can introduce in nitro and keep in Nuxt until the 2.10 release so we don't diverge either for nitro what I suggest:

  • Introduce (runtime) updateRuntimeConfig() that adds a new override object initialized empty in nitro (can be ported but I would wait for 2.10 syncing behavior properly)
  • Introduce (build time) useRuntimeConfig() that shares the same runtime behavior (can be ported to nuxt ahead of time)
  • Introduce (build time) updateRuntimeConfig() that modifies nitro.options.runtimeConfig and triggers an HMR event via nitro.updateConfig() (can be ported to nuxt ahead of time)

@danielroe
Copy link
Member Author

Thanks for the clear explanation. Yes, what was in my mind was purely about build-time utilities. The change from process.env was not meant to affect runtime behaviour, as these utilities were not being exposed within the runtime/ utils. (But very happy with your proposed runtime override solution!)

(The kind of usage I'm imagining was normalising nuxt.options.runtimeConfig - but I guess your suggestion of useRuntimeConfig and updateRuntimeConfig might be safer. Is that your take?)

@pi0
Copy link
Member

pi0 commented May 9, 2024

Splitting into subtopics:

I strongly belive in making a new source/definition of environment variables (as proposed with opts.env) in this PR can be unsafe but also understand there are cases like testing that need it and love to discuss how to approach it better.

@danielroe
Copy link
Member Author

Thank you! One more use-case to mention for allowing non-process environment is to 'preview' the resolved config if environment variables were to be set. I'm very much not proposing using arbitrary overrides for the actual resolved Nitro runtime config, just trying to make the underlying logic reusable in different contexts so it doesn't have to be duplicated.

@pi0
Copy link
Member

pi0 commented May 9, 2024

to 'preview' the resolved config

I guess at least all common runtimes allow to modify environment variables by directly modifying their env object (Deno.env.set and process.env object) if testing the exact env mapping for runtime-config is a must-have in a scenario. (*) and it is safer because:

The benefit of keeping runtime-config behavior strictly based on one standard (**) source (that is what environment variable in the runtime context means) is, that environment variables and how runtime config implicitly works with them won't diverge in a project that uses both environment variables and runtime config, behavior is consistently same. It makes tests more reliable too and also build and runtime utils consistent.


(*) I understand that today, for testing modules with different sets of runtime configs, we strictly need this and also updating global env looks more hacky but let's consider that 1) updateRuntimeConfig didn't exist and 2) updating an env in test/dev isolates gives the safety reason above and allowing to override runtime config if needed.

(**) Today, we have already multiple definitions of environment variables: Actual system environment variables (that itself yet has no unified standard to access), .env augmentations, bundler bundled environment variables and strange runtimes like cloudflare that combine them with other values and make accessible in second stages. I belive both us and test runners have to help to at least keep on these known things rather than introducing more ways to change env variable access behavior and small places like this where we have alternatives that helps to avoid adding new source (even as opt-in even as for test/preview it will still add to less-consistent behavior)

@pi0
Copy link
Member

pi0 commented May 9, 2024

@danielroe Shall we close PR and later continue discussing about this perhaps after updateRuntimeConfig util landed? πŸ™πŸΌ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants