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: add missing sw-language-id header #1657

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

fix: add missing sw-language-id header #1657

wants to merge 13 commits into from

Conversation

mdanilowicz
Copy link
Collaborator

@mdanilowicz mdanilowicz commented Jan 30, 2025

Description

This pull request includes several changes to add the missing sw-language-id header property for API calls across multiple components and modules. The most important changes are grouped by theme as follows:

API Header Updates:

  • Added sw-language-id header property for API calls in useCart, useCategorySearch, useCheckout, useCustomerOrders, useInternationalization, and useLandingSearch composables. [1] [2] [3] [4] [5] [6]

Import and Context Updates:

  • Updated imports to include useSessionContext in various files to access languageIdChain. [1] [2] [3] [4] [5] [6]

Test Updates:

  • Updated tests to include the sw-language-id header property for API call expectations in useCart, useCategorySearch, useInternationalization, and useListing components. [1] [2] [3] [4]

Component-Specific Changes:

  • Added sw-language-id header property in SwContactForm.vue and App.vue components. [1] [2]

closes #1651

Type of change

ToDo's

Screenshots (if applicable)

Additional context

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
frontends-demo 🔄 Building (Inspect) Visit Preview Feb 9, 2025 8:57pm
1 Skipped Deployment
Name Status Preview Updated (UTC)
shopware-frontends-docs ⬜️ Skipped (Inspect) Feb 9, 2025 8:57pm

Copy link

pkg-pr-new bot commented Jan 30, 2025

Open in Stackblitz

@shopware/api-gen

npm i https://pkg.pr.new/shopware/frontends/@shopware/api-gen@1657

@shopware/cms-base-layer

npm i https://pkg.pr.new/shopware/frontends/@shopware/cms-base-layer@1657

@shopware/composables

npm i https://pkg.pr.new/shopware/frontends/@shopware/composables@1657

@shopware/helpers

npm i https://pkg.pr.new/shopware/frontends/@shopware/helpers@1657

@shopware/nuxt-module

npm i https://pkg.pr.new/shopware/frontends/@shopware/nuxt-module@1657

@shopware/api-client

npm i https://pkg.pr.new/shopware/frontends/@shopware/api-client@1657

commit: 3c8c2aa

@mkucmus
Copy link
Collaborator

mkucmus commented Jan 30, 2025

why does API ignore the current session language set by PATCH /context and we need to pass it explicitly?

@mdanilowicz
Copy link
Collaborator Author

why does API ignore the current session language set by PATCH /context and we need to pass it explicitly?

It's not ignoring. The second tab request overrides this session language by a PATCH /context request, and when you get back to the first tab, you have language from the second tab. That is why we have to use this header

Copy link
Collaborator

@mkucmus mkucmus left a comment

Choose a reason for hiding this comment

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

looks like a lot of work done down here 🔥 ⛑️ 🏅

TODO:

  • fix breadcrumbs for navigation because it still mixes the languages (when I refresh the page on the different language in a second tab - breadcrumbs at the first tab is in a wrong language)

TO CONSIDER:

  • as languageIdChain is broadly used now, it still tells nothing and the name might be confusing for developers, maybe we can alias it to something more clear and fancy?
  • maybe we can hook-in the apiClient instance and set sw-language-id globally for entire app?

@mdanilowicz mdanilowicz self-assigned this Feb 4, 2025
@mdanilowicz
Copy link
Collaborator Author

  • fix breadcrumbs for navigation because it still mixes the languages (when I refresh the page on the different language in a second tab - breadcrumbs at the first tab is in a wrong language)

I'm not able to reproduce the problem with breadcrumbs. Are you sure that this is not a problem with the missing translations? Please add steps to reproduce

  • as languageIdChain is broadly used now, it still tells nothing and the name might be confusing for developers, maybe we can alias it to something more clear and fancy?

sure, why not

  • maybe we can hook-in the apiClient instance and set sw-language-id globally for entire app?

Not all endpoints consume sw-language-id. So, adding it globally can be misleading

@mkucmus
Copy link
Collaborator

mkucmus commented Feb 5, 2025

  • fix breadcrumbs for navigation because it still mixes the languages (when I refresh the page on the different language in a second tab - breadcrumbs at the first tab is in a wrong language)

I'm not able to reproduce the problem with breadcrumbs. Are you sure that this is not a problem with the missing translations? Please add steps to reproduce

  • as languageIdChain is broadly used now, it still tells nothing and the name might be confusing for developers, maybe we can alias it to something more clear and fancy?

sure, why not
💪🏼

  • maybe we can hook-in the apiClient instance and set sw-language-id globally for entire app?

Not all endpoints consume sw-language-id. So, adding it globally can be misleading

you are adding it globally but behind each composable function's logic.
If API doesn't consume the header, it will be simply ignored, right? so it would be just a code lines saver.

If the context has its language set, the client could be aware of it in a global state to avoid that some requests won't be in a right language - within the whole composables package. @patzick is that improvement or shadowing the whole thing with translations and current context? or it's just not possible though 😆

@mdanilowicz
Copy link
Collaborator Author

Ok, I see, fixed and added a task for backend shopware/shopware#6630

@mkucmus, but please remember that header is not the only way to set the language. We also use context to do it. I'm afraid that we can overcomplicate it, but I'm not totally against global header :D

Copy link

codspeed-hq bot commented Feb 6, 2025

CodSpeed Performance Report

Merging #1657 will not alter performance

Comparing fix/GH-1651 (3c8c2aa) with main (09e02c5)

Summary

✅ 9 untouched benchmarks

@mkucmus
Copy link
Collaborator

mkucmus commented Feb 6, 2025

Ok, I see, fixed and added a task for backend shopware/shopware#6630

@mkucmus, but please remember that header is not the only way to set the language. We also use context to do it. I'm afraid that we can overcomplicate it, but I'm not totally against global header :D

yup, it's already overcomplicated because we have dynamic content, reactive, bound to the Id and we synchronize the session between browser tabs - that leads to the problem we encounter.

Let's do the rework of that in the future, starting from the backend to ensure that the global sw-language-id header can be applied for every endpoint.

@BrocksiNet
Copy link
Collaborator

Let's do the rework of that in the future, starting from the backend to ensure that the global sw-language-id header can be applied for every endpoint.

This should be collected and driven by the API working group. 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

[BUG] Language context is broken for the multi language tabs
3 participants