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

Ajout du support des sourcemaps pour le JS et le (S)CSS #3171

Merged
merged 2 commits into from
Nov 21, 2015

Conversation

sandhose
Copy link
Contributor

@sandhose sandhose commented Nov 8, 2015

Q R
Correction de bugs ? non
Nouvelle Fonctionnalité ? ~oui
Tickets (issues) concernés reprise de #2848

Les sourcemaps sont des fichiers (.map) qui vont contenir les sources à la base des fichiers CSS et JS. En gros, ca permet dans l'inspecteur web de naviguer directement dans le code "source", et pas dans le code généré/minimifié. A noter que ca rajoute rien comme taille au fichier de base, puisque la sourcemap générée se trouve à chaque fois dans un fichier à part.

La où ça va être vachement cool, c'est qu'on va pouvoir avoir en prod les sources en inspectant l'élément, et donc entre-autre ajouter correctement des breakpoints JS pour debugger, sans pour autant rajouter du temps de chargement (puisque les sourcemaps se chargent que si l'inspecteur est ouvert)


Voila en gros ce que ça donne sur Chrome:

image

image

image


Instructions QA

  • Mettre à jour le front + le builder
  • Vérifier que les sourcemaps sont bien pris en compte:
    • ... dans les sources JS, en rajoutant un breakpoint dans un des fichiers sources (cf. screenshot)
    • ... en inspectant un élément, en vérifiant que le numéro de ligne/fichier correspond bien au fichier source, et non le fichier compilé

Testé sur Chrome et Firefox ; après, c'est possible qu'il faille regarder dans les options du navigateur pour activer les sourcemaps si ça marche pas. Aussi, les sourcemaps sont pas aussi bien gérées sur Firefox que sur Chrome, et les noms relatifs sont pas gérés, ce qui donne des chemins de ce style:
image

Ne pas merger tout de suite !

Cette PR déprécie les versions < 0.12 de node, et donc risque de casser le front pour ceux qui sont encore sous cette version. Je vais mettre à jour la doc encore pour donner les instructions pour installer une version récente de node via le dépot de nodesource, sachant que la le stack est compatible même avec les versions 4.x et 5.x de nodejs.

@Eskimon Eskimon added Evolution C-Front Concerne l'interface du site labels Nov 9, 2015
@Situphen
Copy link
Member

Veto ! Il y a la même erreur que j'ai eu sur #2848 ! Si tu regardes .cookies-eu-banner et .accessibility, tu verras que le nom et le contenu des fichiers SCSS qui leur sont rattachés ne correspondent pas.

Par contre, le bug des multiples requêtes faites pas Firefox a l'air d'avoir disparu.

@sandhose
Copy link
Contributor Author

Euh, non ?
image
image

@sandhose
Copy link
Contributor Author

Il y a effectivement le bug sur FF. La question c'est est-ce que c'est le sourcemap le fautif, ou Firefox ?

@sandhose
Copy link
Contributor Author

Alors, quand ça marche sur Chrome, Safari, Opéra, IE, et Microsoft Edge, je pense qu'on peut dire que c'est pas notre faute et tant pis pour Firefox ?

@Situphen
Copy link
Member

Je pense aussi oui.

@pierre-24
Copy link
Member

Bon, on merge ou pas, du coup ? :)

@gustavi
Copy link
Contributor

gustavi commented Nov 21, 2015

On merge, ça vient clairement de FF là.

@pierre-24
Copy link
Member

Vendu !

pierre-24 added a commit that referenced this pull request Nov 21, 2015
Ajout du support des sourcemaps pour le JS et le (S)CSS
@pierre-24 pierre-24 merged commit 98223d1 into zestedesavoir:dev Nov 21, 2015
@pierre-24 pierre-24 added this to the Version de développement milestone Nov 21, 2015
@sandhose sandhose deleted the sourcemaps branch January 6, 2016 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Front Concerne l'interface du site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants