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

Server til typescript #6038

Open
wants to merge 14 commits into
base: endre-server-auth-til-oasis
Choose a base branch
from

Conversation

johannbm
Copy link
Contributor

Gjør om server til typescript.

Endringer i byggeprosess:

  • Workflow må nå inkludere en yarn build siden ts må transpileres til js for å kunne kjøre på server. legg til yarn build fp-gha-workflows#202

    • Synes det er litt dumt at workflowen som er sydd spesifikt for fp-frontend likevel ligger i felles-workflow mappa? Da må disse to PRene synkroniseres når det gjøres en breaking change som nå. Den er såpass skreddersydd at jeg mener den egentlig burde ligge i dette repoet (?)
  • Før kopierte man over hele server mappe (inkl node_modules++). Serverkode kompileres nå til 1 enkelt fil som er det eneste vi kopiere over til /app i image

  • Image vil nå kun ha en app-folder der det ligger index.js som er server koden, og hvor resten av filene er Frontenden

@johannbm johannbm requested a review from a team as a code owner November 14, 2024 12:12
@@ -0,0 +1,37 @@
import eslint from "@eslint/js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

En ganske streng linter jeg ofte tar med meg

@@ -6,7 +6,10 @@
"type": "module",
"packageManager": "[email protected]",
"scripts": {
"start-express": "node ./server.js"
"build": "ncc build ./src/server.ts -o dist -t --no-cache",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kopiert samme build som brukes i selvbetjening

}
};

const skip = () => process.env.NODE_ENV === "production";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hvorfor skipper vi egentlig? Er det kun relevant å logge med morgan lokalt i autotest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Denne logget primært kallene fra isAlive og isReady - dette ble mye spam i prod så jeg skipper denne der.

`${api.path}/*`,
(request, response, next) => {
if (request.timedout) {
logger.warning(`Request for ${request.originalUrl} timed out!`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Her avdekket blandt annet TS en trolig bug ved at det før logges response.originalUrl istedenfor request.originalUrl


// serve static files
server.use(express.static("./"));
server.use("*", (request, response) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forenklet denne til * istedenfor "ikke dist". Da det uans er siste middleware som vil fallbacke til å serve SPAen

response.sendFile("index.html");
});

// TODO: her var det en error handler. Jeg tror ikke vi trenger da det fanges av default handler: https://expressjs.com/en/guide/error-handling.html
Copy link
Contributor Author

Choose a reason for hiding this comment

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

usikker om vi trengte vår egen errorhandler. Som i lenken ser man at express har sin egen som gjorde det samme

Copy link

sonarcloud bot commented Nov 14, 2024

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.

2 participants