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: locale polluted by cross request #39

Merged
merged 4 commits into from
Nov 19, 2024

Conversation

kongmoumou
Copy link
Contributor

Description

Currently, when there are two concurrent requests with two different locales, the locale of the first locale would be overridden by the second request because they share the same i18n context but in different h3 event context. This PR fix it by binding the locale of h3 event to translate function.

Before

image

After

Test pass ✅

Linked Issues

Additional context

@kongmoumou
Copy link
Contributor Author

@kazupon Could u take some time to review this PR? Thx a lot!

@kazupon kazupon added bug Includes new features ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf labels Nov 19, 2024
src/index.ts Outdated
@@ -27,6 +33,7 @@ import type {
declare module 'h3' {
interface H3EventContext {
i18n?: CoreContext
i18nLocale?: LocaleDetector
Copy link
Member

Choose a reason for hiding this comment

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

i18nLocale is for internal use, so we don't want to expose it to H3EventContext. Please don't export it. 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The LocaleDetector getter needs to bind to event scope. Maybe rename it to _i18nLocale, which means that it's private. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename it to _i18nLocale, which means that it's private. 🤔

Look good to me!
You can change to it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done~

Copy link
Member

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! 💚
Good catch!
I've reviewed your PR
Please check it!

Copy link
Member

@kazupon kazupon left a comment

Choose a reason for hiding this comment

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

Thank you!

@kazupon kazupon merged commit d5589ef into intlify:main Nov 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Includes new features ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants