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(deps): upgrade next to v12 #4280

Merged
merged 34 commits into from
Mar 22, 2022
Merged

fix(deps): upgrade next to v12 #4280

merged 34 commits into from
Mar 22, 2022

Conversation

maxgfr
Copy link
Member

@maxgfr maxgfr commented Mar 3, 2022

fix #4044

  • use rust compiler swc for next
  • use rust compiler swc for jest
  • remove server
  • use next linter
  • set latest version of react
  • merge of redirect.json between koa method and new method (finalement non)
  • handle robots.txt per env
  • fix variable NEXT_PUBLIC_IS_PRODUCTION_DEPLOYMENT by using docker build args
  • remove unused file

@maxgfr maxgfr changed the title fix(deps): upgrade next to v12 WIP fix(deps): upgrade next to v12 Mar 3, 2022
Comment on lines 49 to 64
{
newPath: "information",
newValue: "suppression-de-lattestation-garde-denfant",
path: ["modeles-de-courriers"],
previousValues: [
"attestation-sur-lhonneur-arret-de-travail-pour-la-garde-denfant",
],
},
{
newValue:
"ministere-du-travail-notre-dossier-sur-le-coronavirus/nouveautes-covid19",
path: ["dossiers"],
previousValues: [
"ministere-du-travail-notre-dossier-sur-le-coronavirus/ce-que-changent-les-ordonnances-en-droit-du-travail",
],
},
Copy link
Member Author

Choose a reason for hiding this comment

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

c'est pour le redirect.json du serveur

Comment on lines 29 to 31
if (newPath) {
res = res.replace(v, newPath);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

pour switcher de path

Comment on lines +41 to +64
jest.mock("@cdt/data...prime-precarite/precarite.data.json", () => [
{ criteria: { bar: "baz", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "baz", foo: "2| baz" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "2| baz" }, idcc: "10" },
{
allowBonus: false,
criteria: { foo: "3| bar" },
endMessage: "nope",
hasConventionalProvision: true,
idcc: "20",
},
{
allowBonus: true,
criteria: { foo: "4| baz" },
hasConventionalProvision: true,
idcc: "20",
},
{
criteria: {},
hasConventionalProvision: null,
idcc: "30",
},
]);
Copy link
Member Author

Choose a reason for hiding this comment

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

j'étais obligé de faire ça car ccnList n'est pas instancié, c'est bizarrrrree

Copy link
Contributor

Choose a reason for hiding this comment

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

chelou

Copy link
Contributor

Choose a reason for hiding this comment

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

Le problème est que l'on importe import data from "@cdt/data...prime-precarite/precarite.data.json"; puis derrière, on fait un jest.mock("@cdt/data...prime-precarite/precarite.data.json". Par contre, à part split le fichier en 2, je n'ai pas de solution...

@@ -46,4 +46,20 @@ export const MappingReplacement = [
path: ["information"],
previousValues: ["vaccination-et-pass-sanitaire-les-dates-a-retenir"],
},
{
newPath: "information",
Copy link
Member Author

Choose a reason for hiding this comment

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

c'est ça la newPath

Comment on lines 53 to +59
/>
<script key="polyfill" src="/static/polyfill.min.js" />
<script
<Script key="polyfill" src="/static/polyfill.min.js" />
<Script
key="webcomponents"
src="/static/webcomponents-polyfill/loader.js"
/>
<script key="smarttag" src="/static/smarttag.js" />
<Script key="smarttag" src="/static/smarttag.js" />
Copy link
Member Author

Choose a reason for hiding this comment

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

next n'aime pas avoir ça là, mais j'ai hacké le truc

Copy link
Member Author

Choose a reason for hiding this comment

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

il faudrait l'avoir sur les pages

Copy link
Member Author

Choose a reason for hiding this comment

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

mais pas dans le document

@@ -1,6 +0,0 @@
root: true
Copy link
Member Author

Choose a reason for hiding this comment

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

ça faisait bugguer le nouveau linter de next

Copy link
Member Author

Choose a reason for hiding this comment

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

et j'ai pas vu de changement sur les autres repos

@@ -1 +0,0 @@
packages/code-du-travail-frontend/public/static
Copy link
Member Author

Choose a reason for hiding this comment

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

je vois pas l'utilité hmm

docs
doc
website
images
Copy link
Member Author

Choose a reason for hiding this comment

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

ça faisait bugguer le nouveau compiler

"precommit": "lint-staged",
"prepush": "yarn test --bail --changedSince=master",
"start": "NODE_ENV=production node server/server.js",
"start": "NODE_ENV=production next start",
"prebuild": "node -r @swc-node/register scripts/prebuild.ts",
Copy link
Member Author

Choose a reason for hiding this comment

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

c'est plus rapide que ts-node

.next

robots.txt
Copy link
Member Author

Choose a reason for hiding this comment

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

en gros le prebuild, genere le robots.txt, il faudrait que je vois que si le dockerfile nous permet de generer le bon robots.txt au niveau de la prod et de la preview

@@ -14,6 +12,47 @@ const withBundleAnalyzer = require("@next/bundle-analyzer")({
enabled: process.env.ANALYZE === "true",
});

const csp = {
Copy link
Member Author

Choose a reason for hiding this comment

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

c'est ici les csp

Copy link
Contributor

Choose a reason for hiding this comment

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

quand tu test en local, tu peux regarder si t'ajoute une image depuis une url externe qu'elle est bien bloquée stp ?

Comment on lines +128 to +132
{
destination: "/api/health",
permanent: true,
source: "/health",
},
Copy link
Member Author

Choose a reason for hiding this comment

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

pour garder le endpoint health qui renvoie un json ok

@@ -15,7 +15,7 @@ export default function Custom404() {
return (
<>
<Head>
<Metas title="Page non trouvée" />
<Metas title="Page non trouvée" description="Page 404" />
Copy link
Member Author

@maxgfr maxgfr Mar 7, 2022

Choose a reason for hiding this comment

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

une description qui manqué au niveau de la page 404 et qui m'a été souligné, du coup j'ai mis ça mais on peut mettre une chaine de caractère vide

Comment on lines 43 to 44
${"https://code.travail.gouv.fr/modeles-de-courriers/attestation-sur-lhonneur-arret-de-travail-pour-la-garde-denfant"} | ${"https://code.travail.gouv.fr/information/suppression-de-lattestation-garde-denfant"}
${"https://code.travail.gouv.fr/dossiers/ministere-du-travail-notre-dossier-sur-le-coronavirus/ce-que-changent-les-ordonnances-en-droit-du-travail"} | ${"https://code.travail.gouv.fr/dossiers/ministere-du-travail-notre-dossier-sur-le-coronavirus/nouveautes-covid19"}
Copy link
Member Author

Choose a reason for hiding this comment

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

les deux nouveaux tu pour le merge du redirect.json du serveur et de notre approche locale

Copy link
Contributor

Choose a reason for hiding this comment

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

les 2 urls n'existent plus... donc y'avait pas besoin de les bouger en fait

Comment on lines 111 to 113
process.env.NEXT_PUBLIC_IS_PRODUCTION_DEPLOYMENT !== "true"
? "noindex, nofollow, nosnippet"
: "all",
Copy link
Member Author

Choose a reason for hiding this comment

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

pour ne pas indexer le robots.txt

j'ai essayé de l'écrire comme cela:

...(process.env.NEXT_PUBLIC_IS_PRODUCTION_DEPLOYMENT !== "true" && {
  key: "x-robots-tag",
  value: "noindex, nofollow, nosnippet",
}),

mais cela n'a pas fonctionné, je l'ai mais en all pour la prod d'après la doc :

https://developers.google.com/search/docs/advanced/robots/robots_meta_tag

@maxgfr maxgfr marked this pull request as ready for review March 7, 2022 17:07
@maxgfr maxgfr requested a review from a team as a code owner March 9, 2022 15:52
Copy link
Contributor

@carolineBda carolineBda left a comment

Choose a reason for hiding this comment

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

Vraiment bien joué, bravo pour cette PR !

value: ContentSecurityPolicy.replace(/\n/g, " ").trim(),
},
{
key: "X-Robots-Tag",
Copy link
Contributor

Choose a reason for hiding this comment

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

A verifier en prod

const withBundleAnalyzer = require("@next/bundle-analyzer")({
enabled: process.env.ANALYZE === "true",
});

const ContentSecurityPolicy = `
Copy link
Contributor

Choose a reason for hiding this comment

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

A vérifier en prod

@@ -15,7 +15,10 @@ export default function Custom404() {
return (
<>
<Head>
<Metas title="Page non trouvée" />
<Metas
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

[
{
"baseUrl": "/modeles-de-courriers/attestation-sur-lhonneur-arret-de-travail-pour-la-garde-denfant",
"code": 303,
Copy link
Contributor

Choose a reason for hiding this comment

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

je sais pas pourquoi ici on avant un 303. Chelou

Comment on lines 43 to 44
${"https://code.travail.gouv.fr/modeles-de-courriers/attestation-sur-lhonneur-arret-de-travail-pour-la-garde-denfant"} | ${"https://code.travail.gouv.fr/information/suppression-de-lattestation-garde-denfant"}
${"https://code.travail.gouv.fr/dossiers/ministere-du-travail-notre-dossier-sur-le-coronavirus/ce-que-changent-les-ordonnances-en-droit-du-travail"} | ${"https://code.travail.gouv.fr/dossiers/ministere-du-travail-notre-dossier-sur-le-coronavirus/nouveautes-covid19"}
Copy link
Contributor

Choose a reason for hiding this comment

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

les 2 urls n'existent plus... donc y'avait pas besoin de les bouger en fait

Comment on lines +41 to +64
jest.mock("@cdt/data...prime-precarite/precarite.data.json", () => [
{ criteria: { bar: "baz", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "baz", foo: "2| baz" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "2| baz" }, idcc: "10" },
{
allowBonus: false,
criteria: { foo: "3| bar" },
endMessage: "nope",
hasConventionalProvision: true,
idcc: "20",
},
{
allowBonus: true,
criteria: { foo: "4| baz" },
hasConventionalProvision: true,
idcc: "20",
},
{
criteria: {},
hasConventionalProvision: null,
idcc: "30",
},
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

chelou

Copy link
Contributor

@m-maillot m-maillot left a comment

Choose a reason for hiding this comment

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

👍


jest.mock("fs");

describe("robots.txt", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

😍😍😍

`Sitemap: https://${host}/sitemap.xml`,
].join("\n");
generateRobotsTxt(true, host);
expect(fs.writeFileSync).toHaveBeenCalledWith(filePath, robotsProd);
Copy link
Contributor

Choose a reason for hiding this comment

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

ça serait bien de vérifier le filepath, pour éviter de le changer et qu'il se retrouve au mauvais endroit :)

Comment on lines +41 to +64
jest.mock("@cdt/data...prime-precarite/precarite.data.json", () => [
{ criteria: { bar: "baz", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "1| foo" }, idcc: "10" },
{ criteria: { bar: "baz", foo: "2| baz" }, idcc: "10" },
{ criteria: { bar: "bar", foo: "2| baz" }, idcc: "10" },
{
allowBonus: false,
criteria: { foo: "3| bar" },
endMessage: "nope",
hasConventionalProvision: true,
idcc: "20",
},
{
allowBonus: true,
criteria: { foo: "4| baz" },
hasConventionalProvision: true,
idcc: "20",
},
{
criteria: {},
hasConventionalProvision: null,
idcc: "30",
},
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Le problème est que l'on importe import data from "@cdt/data...prime-precarite/precarite.data.json"; puis derrière, on fait un jest.mock("@cdt/data...prime-precarite/precarite.data.json". Par contre, à part split le fichier en 2, je n'ai pas de solution...

frame-src 'self' https://mon-entreprise.urssaf.fr https://matomo.fabrique.social.gouv.fr *.dailymotion.com;
style-src 'self' 'unsafe-inline';
font-src 'self' data: blob:;
prefetch-src 'self' *.fabrique.social.gouv.fr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

je l'ai rajouté le prefetch en pensant que ça reglerait, c'est un test

Copy link
Member Author

Choose a reason for hiding this comment

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

et effectivement c'est ce qu'il fallait rajouter

Copy link
Contributor

Choose a reason for hiding this comment

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

ça fix un warning qu'on voyant dans la console c'est ça ?

const ContentSecurityPolicy = `
default-src 'self' *.travail.gouv.fr *.data.gouv.fr *.fabrique.social.gouv.fr;
img-src 'self' data: *.fabrique.social.gouv.fr https://travail-emploi.gouv.fr https://mon-entreprise.urssaf.fr ${
process.env.AZURE_BASE_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

cette var est undefined :
voila le header qu'on a :
img-src 'self' data: *.fabrique.social.gouv.fr https://travail-emploi.gouv.fr https://mon-entreprise.urssaf.fr undefined;

Copy link
Member Author

@maxgfr maxgfr Mar 18, 2022

Choose a reason for hiding this comment

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

Ahhh en effet, quand c'est non defini on met undefined, il faudrait faire

Suggested change
process.env.AZURE_BASE_URL
${process.env.AZURE_BASE_URL ?? ""}

const ContentSecurityPolicy = `
default-src 'self' *.travail.gouv.fr *.data.gouv.fr *.fabrique.social.gouv.fr;
img-src 'self' data: *.fabrique.social.gouv.fr https://travail-emploi.gouv.fr https://mon-entreprise.urssaf.fr ${
process.env.AZURE_BASE_URL ?? ""
Copy link
Contributor

Choose a reason for hiding this comment

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

non c'est bon il faut que cette var fonctionne

Copy link
Contributor

@carolineBda carolineBda left a comment

Choose a reason for hiding this comment

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

fix process.env.AZURE_BASE_URL

@github-actions
Copy link

@maxgfr maxgfr requested a review from carolineBda March 21, 2022 08:57
Copy link
Contributor

@carolineBda carolineBda left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@m-maillot m-maillot merged commit 10ae94f into master Mar 22, 2022
@m-maillot m-maillot deleted the maxgfr/upgrade-next branch March 22, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade next js
4 participants