Tous les attributs du composant Link passent en path params.

dylanduclos commented 5 years ago

On observe sur ce code (issu du fichier link.container.js) que tous les attributs du composant Link qui sont différents des attributs onClick, query et code, sont poussés dans l'url comme path params :

const mapDispatch = (
  dispatch,
  {
    onClick,
    code,
    query,
    ...params
  },
) => ({
  onClick: (e) => {
    // parent onClick callback
    if (onClick) onClick(e)

    // sometimes we want to ignore click
    if (shouldIgnoreClick(e)) return

    // dispatch the push
    dispatch(actions.push(code, params, query))

    // prevent default behaviour
    e.preventDefault()
  },
})

C'est plutôt rigide comme fonctionnement car dans beaucoup de cas, il y a des attributs que l'on désire mais qui n'ont rien à voir avec les path params.

Solution potentielle : Tout comme pour les query params, spécifier un attribut pathParams comme ceci : pathParams={ { id: '1' } } par exemple et qui est transmis au container en lieu et place de ...params. De cette manière on spécifie bien dans l'instance de notre Link, ce qui doit et ce qui ne doit pas être considéré comme path params.

fabienjuif commented 5 years ago

Salut Dylan, quand on peut / si tu peux, on préfèrera l'anglais pour parler sur les différents repos alakarteio. :)

Je comprends ton discours mais j'ai du mal à me projeter, peux-tu donner un exemple d'utilisation de Link où tu aurais besoin des props ?

fabienjuif commented 5 years ago

@dylanduclos que penses-tu de faire ça:

<Link code="user" id="2" name="lost" />

génère un <a href= /> comme tu le connais actuellement
la prop name est perdue

<Link code="user" id="2">
  {href => (
    <a href={href}>lost</a>
  )}
</Link>

génère un href mais ne génère pas le "a" c'est à doit de le donner
tu peux utiliser tes props customs comme tu veux

Les 2 API peuvent cohabiter et ça permet de surcharger tout ce que tu veux. Ca permet également de ne pas faire un breaking change pour les projets utilisant déjà le Link comme il est définit.

dylanduclos commented 5 years ago

MAJ : nous avons modifié notre code pour nous adapter au Link que vous avez implémenté.

Pour te donner le contexte, nous avons sur notre projet un composant Button qui peut prendre plusieurs forme selon les propriétés qui lui sont passées. Par exemple, si un attribut href est défini alors le button sera une balise 'a', si un code lui est passé, ce sera un Link et si aucun de es deux attributs n'existe, ce sera un button.

Notre soucis était que le composant était trop 'passe plat' et ne filtrait pas les props qui était passées. On se retrouvait ainsi avec un Link ayant des props qui ne le concernaient pas directement et surtout qui était consommées en amont (dans Button). Cela était gênant car toutes ces props étaient donc considérées comme path params, ce que l'on voulait éviter.

C'est pourquoi le composant Button a été modifié pour être moins passe plat et ne passer au link que les attributs nécessaires.

Mais dans l'idée, avoir à définir un attribut pathParams, tout comme pour les query params qui sont spécifiés dans l'attribut query, aurait offert plus de flexibilité au attributs que l'on peut passer à Link.

Ps : sorry mais je n'avais pas vraiment de code "léger" sous la main pour exposer mon problème car nos Link sont pour la plupart générer par ce composant Button donc la stack de fichiers impliqués dans ce problème est plutôt conséquente ^^

fabienjuif commented 5 years ago

Merci pour les informations.

Concernant le fait d'avoir un fonctionnement plus souple, as-tu regardé ma proposition dans mon second commentaire ?

Pour le fait de passer les pathparam dans un prop à part, c'était un choix délibéré de les passer comme prop au composant :

on utilise quasi tout le temps les path param
c'est verbeux de créer un nouvel objet pour passer quelques path param
je suis pas à l'aise de créer un nouvel objet à chaque rendu...

En ce qui concerne plus spécifiquement votre problème, une règle que je m'applique en react c'est de ne pas faire de <qqch {...props} /> sur des composants "bas niveaux", parceque par exemple, si vous faites ça sur un a ou un button, ça peut poser des soucis également (d'ailleurs je crois que React crache des warnings non s'il ne connait pas les props sur les composants DOM ? Je ne sais plus)

Pour en revenir au path params, le choix est pas évident. Si j'ai d'autre retours qui vont dans ton sens on pourra changer éventuellement. Mais là maintenant je me dis que le meilleurs compromis reste ce que je propose dans mon second commentaire (compromis entre souplesse ET ne pas faire de breaking changes)

Merci pour tes retours 👍 Hésites pas à rebondir si tu n'es pas d'accord 😉

dylanduclos commented 5 years ago

Oui j'ai bien vu ton commentaire et la solution que tu proposes, je t'en remercie par ailleurs, mais ce n'est pas ce type de comportement que nous cherchions. L'idée n'était pas de hard coder un élément enfant qui prendrait les props de Link qui n'ont pas vocation à être des path params.

Mais cela n'est pas bien grave car comme tu l'as souligné, nous avons modifier notre Button pour le rendre moins passe plat et spécifier, à chaque fois qu'il est utiliser, les props que l'on désire réellement passer au lieu d'écrire ...props.

Concernant vos motivations, je les comprends et elles sont légitimes. Tout est une question de point de vue lors de la conception et j'ai bien cerné la vôtre. Nous avons résolu notre problème de notre côté donc , en effet, évitons les breaking changes sauf si ce retour vous est effectivement fait à maintes reprises.

Merci pour votre réactivité les gars en tout cas, vos librairies sont bien sympa et nous économisent pas mal de lignes de codes ;)

A plus !

fabienjuif commented 5 years ago

Pas de problème, merci pour les échanges 👍 Hésitez pas à ouvrir d'autres issues si besoin ;)

unirakun / k-redux-router

Tous les attributs du composant Link passent en path params. #63