Skip to content
This repository has been archived by the owner on Dec 26, 2022. It is now read-only.

Ajout de fichiers pour gérer des dépendances de développement #51

Closed
wants to merge 3 commits into from

Conversation

julienw
Copy link
Collaborator

@julienw julienw commented May 24, 2021

Fixes #49

J'ai vérifié: node_modules est automatiquement ignoré par web-ext. En revanche package.json, yarn.lock ou même README.md sont dans le paquet généré. Est-ce un souci ?

Je ne suis pas convaincu par l'usage de yarn. Je l'ai utilisé par habitude, mais ça ajoute une étape lors de l'installation du projet... Vous en pensez quoi ?

Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Effectivement utiliser NPM simplifierait un peu l'installation. A mon avis autant exclure tous les fichiers que tu as mentionné pour le moment.

@julienw
Copy link
Collaborator Author

julienw commented May 24, 2021

Effectivement utilise NPM simplifierait un peu l'installation. A mon avis autant exclure tous les fichiers que tu as mentionné pour le moment.

OK, je vais remplacer par npm, et ajouter un script build avec les ignore pour que tu puisses l'utiliser pour la release.
D'ailleurs tu fais comment ta release en général ? D'abord build puis sign ? Ou bien sign fait un build aussi de manière implicite ? Ou bien tu build puis laisses le site web addons.mozilla.org faire la signature ?

@dunglas
Copy link
Owner

dunglas commented May 25, 2021

Je fais juste un build puis j'upload sur addons.mozilla.org qui s'occupe de la signature.

@julienw julienw force-pushed the use-dev-dependencies branch 3 times, most recently from 9e6157f to 612d692 Compare May 26, 2021 16:50
Comment on lines +17 to +22
"webExt": {
"ignoreFiles": [
"*.md",
"package*.json"
]
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'ai découvert ça, c'est bien pratique pour avoir des options utilisées pour toutes les commandes et avoir du coup des commandes plus claires :-)

package.json Outdated
"lint": "prettier --check .",
"lint-fix": "prettier --check -w .",
"start": "web-ext run",
"build": "web-ext build --overwrite-dest"
Copy link
Collaborator Author

@julienw julienw May 26, 2021

Choose a reason for hiding this comment

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

j'ai laissé le --overwrite-dest sur la ligne de commande plutôt que le mettre dans l'objet de config ci-dessous, car je me suis dit que tu voudrais peut-être pas de ce comportement si tu exécutes web-ext directement. Mais si tu préfères, je peux le déplacer.

Copy link
Owner

Choose a reason for hiding this comment

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

Je ne mettrai pas du tout cette option perso, c'est mieux d'être sûr de ce qu'on fait quand on build.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comme tu veux, c'est toi qui buildes après tout ;-)

@julienw julienw force-pushed the use-dev-dependencies branch 3 times, most recently from 9828586 to 92429f8 Compare May 26, 2021 16:57
@julienw
Copy link
Collaborator Author

julienw commented May 26, 2021

@dunglas je veux bien un second check, et surtout que tu essaies les différents trucs chez toi pour dire si ça te va :-)

@julienw julienw requested a review from dunglas May 26, 2021 16:58
Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Autant utiliser l'installe locale de web-ext partout non ?

"author": "Kévin Dunglas <dunglas@gmail.com>",
"license": " AGPL-3.0-only",
"scripts": {
"lint": "prettier --check .",
Copy link
Owner

@dunglas dunglas May 26, 2021

Choose a reason for hiding this comment

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

Suggested change
"lint": "prettier --check .",
"lint": "prettier --check . && web-ext lint",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'avais fait ça d'abord, mais je ne suis pas convaincu que ça fonctionne sous Windows, et puis si prettier échoue tu n'as pas le résultat de web-ext lint...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dis moi tout de même si tu préfères ça, et je le fais :-)

Copy link
Owner

@dunglas dunglas May 26, 2021

Choose a reason for hiding this comment

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

T'as raison, ça ne marche pas sous Windows... Je hais l'écosystème JS :D
Du coup je me demande si ces scripts et ces deps ont un réel intérêt comparé à juste indiquer dans le README de lancer npx prettier --write . && npx web-ext lint ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pour le lint JS avec prettier, je trouve ça un peu chiant à taper en entier ;-)
(bon c'est aussi que mon vim n'est pas configuré pour l'exécuter car dans mon projet "normal" on utilise eslint qui lance lui prettier... alors je veux bien plaider coupable)

pour le web-ext je n'avais pas ajouté de script parce que je suis d'accord sur le peu d'intérêt...

Copy link
Owner

Choose a reason for hiding this comment

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

Utilise ça dans ce cas peut être? https://prettier.io/docs/en/watching-files.html

Copy link
Contributor

Choose a reason for hiding this comment

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

J'utilise vim aussi mais il est config pour auto-prettier xD

Sinon je suis + pour rester simple et utiliser seulement les outils avec npx, garder le côté léger et fonctionnel du projet

Comment on lines 21 to 23
- name: Lint the web extension
run: npx web-ext lint

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- name: Lint the web extension
run: npx web-ext lint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Petit comportement intéressant de npx: il utilise le package local au projet s'il est présent :-)
donc ça utilise bien la version installée dans le projet.

(j'ai appris ça lorsque j'ai fait cette PR, je trouve que yarn est beaucoup plus logique pour ça finalement...)

Copy link
Owner

Choose a reason for hiding this comment

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

Oui je me suis déjà fait avoir sur d'autres projets par ce comportement (pas très intuitif je trouve).

@julienw
Copy link
Collaborator Author

julienw commented May 26, 2021

Je ferai une autre pr demain avec juste un contributing.md, on pourra comparer.

@pirquessa
Copy link
Collaborator

J'ai hate d'avoir cette tache et une meilleure intégration de Prettier :-)

@julienw
Copy link
Collaborator Author

julienw commented May 27, 2021

J'ai fait #57. Ça convient sans doute mieux au projet et aux envies de @dunglas :-)

(et j'ai pas vraiment envie de passer plus de temps là-dessus, je préférerais passer du temps à améliorer le produit pour les utilisateurs :-) )

@julienw julienw closed this May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoir un package.json avec des scripts et dépendances pour aider le dev
4 participants