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: support dynamic app config and runtime config #1154

Merged
merged 6 commits into from
Apr 21, 2023
Merged

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Apr 17, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 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)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

The initial implementation of both runtime-config and app-config are using a deep frozen version of the build-time config so that it is safe to be shared across requests. However, both app-config and runtime-config are by design dynamic configuration sources.

Dynamic Runtime config

When using useRuntimeConfig() in an ambient context, it uses a shared, cloned, and mutable frozen object.

When using the new useRuntimeConfig(event), cloned object will be cached in the request event. Whenever we call the composable, the latest environment variables will be applied. This supports dynamic env updates as well as Cloudflare module/pages environment compatibility (unjs/unenv#95)

Dynamic App Config

When using useAppConfig() in an ambient context, it returns a shared, cloned, and mutable frozen version of the app config. This is mainly for backward compatibility and is not shared with non-ambient versions.

When using useAppConfig(event), it returns a cloned and mutable version of the app config suitable to be dynamically extended per request without leaking context across requests.

Side note: Nuxt can integrate with the new mutable useAppConfig API of nitro by exposing it from the renderer to the rest of the app) and augmenting with next-specific app-config that might be tree-shaken from the nitro side. This way app config can be universally extended. Since useAppConfig from nuxt is already a context-aware composable it access to the event.

Other notes

Thinking today about dynamic config sources, we should have avoided the ambient context usage for such composables. This PR adds a new feature as an enhancement with fully backward compatibility and without breaking changes but i am thinking to slowly starting deprecating ambient usage and replacing it with a better and clear way for exposing that kind of build-time static configurations. Starting by updating docs and maybe a warning once there was the better alternative in place and widely adopted by Nitro and Nuxt.

This implementation is using klona which according to their benchmarks at least is one of the fastest possible solutions. structured clone as alternative, is not possible within workers. A defu based clone (unjs/defu#90) could be used. But i am also thinking to implement change sets as a smart Proxy with lazy cloning per request. If we have it, it can be also useful for smart app-config hydration from server to client and only including changes.

πŸ“ Checklist

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

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #1154 (bfa5f06) into main (6a4e57e) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1154      +/-   ##
==========================================
+ Coverage   76.96%   77.03%   +0.06%     
==========================================
  Files          67       67              
  Lines        6613     6613              
  Branches      719      726       +7     
==========================================
+ Hits         5090     5094       +4     
+ Misses       1522     1517       -5     
- Partials        1        2       +1     

see 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pi0 pi0 marked this pull request as ready for review April 17, 2023 11:56
@pi0
Copy link
Member Author

pi0 commented Apr 17, 2023

However this PR is fully backward/compatible with Nuxt, would be happy to hear feedbacks from @unjs/nuxt team members if there are real concerns about this direction and waiting for a while before moving this forward to edge.

@nuxt-studio
Copy link
Contributor

nuxt-studio bot commented Apr 17, 2023

βœ… Live Preview ready!

Name Edit Preview Latest Commit
nitro Edit on Studio β†—οΈŽ View Live Preview bfa5f06

@pi0
Copy link
Member Author

pi0 commented Apr 18, 2023

Any feedback regarding this @danielroe ?

Copy link
Member

I'm planning to have a look today.

@danielroe
Copy link
Member

I really like this approach conceptually and think it makes sense πŸ‘ Nice work.

@pi0
Copy link
Member Author

pi0 commented Apr 21, 2023

Update: More thinking mutable config with ambient context, can be an unsafe change especially if go unnoticed by developers. For now, ambient context composables, remain frozen (but cloned).

This means that for now, nitro-dev and cloudflare presets, can only leverage the augmented environment variables as long as they explicitly pass event. Other presets, always have augmentation.

This is not final. In next steps, i plan to work on (mentioned above) proxy package that can enable us internally tracking and mutating a frozen public interface. This way we can avoid both clone costs and also internally apply envs even for ambient frozen public config.

@pi0 pi0 merged commit 1d218eb into main Apr 21, 2023
@pi0 pi0 deleted the feat/dynamic-config branch April 21, 2023 10:22
@pi0 pi0 mentioned this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants