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

[TECH] Améliorer la clareté d'un bout de code manipulant window.location (PIX-12266) #8725

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

lego-technix
Copy link
Contributor

@lego-technix lego-technix commented Apr 23, 2024

⚠️ Cette PR va être réécrite pour être plus globale.

🦄 Problème

Le code suivant n’est pas très clair :

@service location;
// …
const { protocol, host } = location;

🤖 Proposition

Mentionner explicitement window.location comme cela a été fait dans https://github.com/1024pix/pix/blob/dev/admin/app/routes/authentication/login-oidc.js#L98-L101 :

@service location;
// …
const { protocol, host } = window.location;

🌈 Remarques

cf. https://developer.mozilla.org/en-US/docs/Web/API/Window/location

💯 Pour tester

Vérifier qu'une connexion par SSO OIDC fonctionne.

@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@@ -87,7 +87,7 @@ export default class LoginOidcRoute extends Route {
}

_getRedirectUri(identityProviderSlug) {
const { protocol, host } = location;
const { protocol, host } = window.location;
Copy link
Contributor

@igorissen igorissen Apr 23, 2024

Choose a reason for hiding this comment

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

Pour les tests, il faut être capable de stub location de Window, ce qui devrait être impossible. Le moyen de le faire c'est de passer par un wrapper (mon-pix/app/services/location.js) qui est ici le service location.

Je serais plus pour déplacer cette modification dans le service location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

location est dans le scope global, et window.location est dans l'objet window disponible dans le scope global. A priori il est donc aussi facile/difficile de stuber quelque chose dans l'objet window que dans le scope global.

Mais effectivement le service location est là et c'est le bon endroit pour centraliser cela. Je vais modifier cette PR pour faire utiliser le service location. Et je proposerai également dans celle-ci l'utilisation de window.location plutôt que location.

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.

None yet

3 participants