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.

Sign up for GitHub

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

Jump to bottom

feat: check access for secret values on runtime configuration when dev #26441

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

feat: check access for secret values on runtime configuration when dev #26441

wants to merge 2 commits into from

Conversation

nopeless
Copy link

🔗 Linked issue

None

❓ 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

I wasted a bit too much time having trouble why server hydration and client view was different until I realized that I was accessing useRuntimeConfig().<some prop> in client as well. This would prevent future issues like that

📝 Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@stackblitz StackBlitz
Copy link

stackblitz bot commented Mar 22, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

danielroe
danielroe reviewed Mar 22, 2024
View reviewed changes
Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

I think this is a great idea! Thank you ❤️

However, I think as written it will warn when these keys are accessed on server side as well given that we are manipulating the config within the SSR renderer.

Maybe just warn on client side? (We could add the guard within the import.meta.client block within app/nuxt.)

@nopeless
Copy link
Author

I think this is a great idea! Thank you ❤️

However, I think as written it will warn when these keys are accessed on server side as well given that we are manipulating the config within the SSR renderer.

Maybe just warn on client side? (We could add the guard within the import.meta.client block within app/nuxt.)

SSR context is untouched. In fact, it exposes even more properties, specifically .nitro (which in my opinion makes more sense, let me know if this behaviour is not what you want)

only SPA context is altered

@nopeless nopeless marked this pull request as draft March 22, 2024 12:14
@nopeless nopeless force-pushed the improve-runtime-config-access-check branch from 118bbc4 to ed14aea Compare March 22, 2024 13:08
@nopeless
Copy link
Author

@danielroe when ssr: true the error is not triggered

@nopeless nopeless marked this pull request as ready for review March 22, 2024 13:15
@nopeless
Copy link
Author

nopeless commented Apr 3, 2024

bump

@danielroe
Copy link
Member

danielroe commented Apr 3, 2024 •
edited

(This PR is waiting for a minor release - probably Nuxt v3.12. It has not been forgotten)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielroe danielroe danielroe left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
3.x enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nopeless @danielroe