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

fix(hooks): enabled use of useAppConfig() and useRuntimeConfig() within request hooks #2483

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

Conversation

boenrobot
Copy link

If the nitro context isn't already initialized, it will be initialized, rather than error.

πŸ”— 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

When calling useAppConfig(event) or useRuntimeConfig(event) from within a hook, the hook errors, because the "nitro" context option is not initialized yet at that time. It does get initialized later, making it available in middleware and route handlers. This PR extends support to include hooks as well, since although the nitro context option is not initialized, the event is available, so one would expect it to work.

πŸ“ Checklist

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

@pi0
Copy link
Member

pi0 commented May 31, 2024

Thanks for PR. Can you please link to a minimal reproduction please? πŸ™πŸΌ

@boenrobot
Copy link
Author

boenrobot commented May 31, 2024

Thanks for PR. Can you please link to a minimal reproduction please? πŸ™πŸΌ

What about the changes in the tests?

Specifically, test/fixture/plugins/hooks.ts and test/fixture/routes/hooks.ts.

That's pretty much as minimal of a reproduction as you can get.

Restore src/runtime/config.ts to its current state (as opposed to the state in this PR), while keeping the tests files as in this PR, and you will see the tests fail. Surrounding the hook handler with try {} catch (e) { console.error(e);} will show you a message like TypeError: Cannot read properties of undefined (reading 'runtimeConfig') (although exact message may depend on env; this example is from local nodejs installation). The output is wrong regardless, which is why I didn't add it into the test.

I have this fairly minimal project of mine:
https://github.com/boenrobot/nuxt-mikro-orm-module

Where I originally encountered this issue, but even that one is not quite as minimal as the test case changes.

…in request hooks

If the nitro context isn't already initialized, it will be initialized, rather than error.
@@ -20,7 +20,7 @@ declare module "h3" {
captureError: CaptureError;
}
interface H3Context {
nitro: {
nitro?: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is already de facto the case, with or without this change.

Any Nitro plugins where builds break due to this change would be breaking the build because the plugin is now required to handle a (potential) problem when they access the "nitro" property. The behavior is not changed, just enforced on a TS level.

Any Nitro plugin that already handles the possibility of nitro being undefined when accessing it would have their builds keep working.

And any Nitro plugin that doesn't use the Nitro property isn't affected at all.

Any plugin that's currently relying on nitro already being instantiated is in fact just asserting it is, when in fact, it may not be, even today, if the code is triggered in a hook.

And Nitro itself handles it in all cases (now including also useRuntimeConfig(event) and useAppConfig(event)).


All of that said, I guess if there's a way to enforce Nitro's event handlers to use a context where "nitro" is not optional, while having hooks take in a context where "nitro" is optional would be best.

I'll see what I can do, and meanwhile, I amended the commit to init the errors array, as I didn't do that before.

…in request hooks

If the nitro context isn't already initialized, it will be initialized, rather than error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants