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

feat: build react-spa frontend #21

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

jean-michelet
Copy link
Contributor

Issue #20

@jean-michelet
Copy link
Contributor Author

I have the beginnings of something, although it breaks the CI... Works locally, I'll take a look in the week.

Any themes in the community to use for styling? I'm a terrible designer ^^

@jean-michelet jean-michelet marked this pull request as draft August 25, 2024 18:29
@jean-michelet
Copy link
Contributor Author

@galvez @onlywei @melroy89 👋

I don't know why but tests in /tests/routes/api fail during CI when I register vite.
I skipped the fastifyVite plugin registration for tests.
Should we make it work? If yes, any feedback?
Should we test the front? Maybe with Cypress or Playground?

@galvez Do you know which fastify contributors have frontend skills to review my work, besides you?

@jean-michelet jean-michelet marked this pull request as ready for review September 14, 2024 11:14
@onlywei
Copy link

onlywei commented Sep 14, 2024

@jean-michelet I shall take a look this evening.

@onlywei
Copy link

onlywei commented Sep 14, 2024

@jean-michelet I cloned this repo jean-michelet:setup-spa and tried to start working on it to see the issue. Please forgive me and I'm not very fluent at setting up databases, but I have encountered the following errors that are preventing me from running npm test. Here is what I have tried so far:

  1. git clone git@github.com:jean-michelet/demo.git
  2. cd demo
  3. npm i
  4. npm test -- results below:
% npm test

> fastify-demo@0.0.0 test
> npm run db:seed && tap --jobs=1 test/**/*


> fastify-demo@0.0.0 db:seed
> node --env-file=.env scripts/seed-database.js

node: .env: not found

Then I tried docker compose up. Results are as follows:

% docker compose up                          
WARN[0000] The "MYSQL_DATABASE" variable is not set. Defaulting to a blank string. 
WARN[0000] The "MYSQL_USER" variable is not set. Defaulting to a blank string. 
WARN[0000] The "MYSQL_PASSWORD" variable is not set. Defaulting to a blank string. 
[+] Running 1/1
 ✔ Container demo-db-1  Recreated                                                                                                                    0.1s 
Attaching to db-1
db-1  | 2024-09-14 23:02:20+00:00 [Note] [Entrypoint]: Entrypoint script for MySQL Server 8.4.2-1.el9 started.
db-1  | 2024-09-14 23:02:21+00:00 [Note] [Entrypoint]: Switching to dedicated user 'mysql'
db-1  | 2024-09-14 23:02:21+00:00 [Note] [Entrypoint]: Entrypoint script for MySQL Server 8.4.2-1.el9 started.
db-1  | 2024-09-14 23:02:21+00:00 [ERROR] [Entrypoint]: Database is uninitialized and password option is not specified
db-1  |     You need to specify one of the following as an environment variable:
db-1  |     - MYSQL_ROOT_PASSWORD
db-1  |     - MYSQL_ALLOW_EMPTY_PASSWORD
db-1  |     - MYSQL_RANDOM_ROOT_PASSWORD
db-1 exited with code 1

Was there a step I am missing?

@melroy89
Copy link
Contributor

.env file missing? That could be a clue. 😅

@onlywei
Copy link

onlywei commented Sep 15, 2024

Was I supposed to manually create my own .env file for this? I didn't see that in the Readme.

@millette
Copy link

@onlywei .env files are usually (always?) private. There's a sample given: https://github.com/fastify/demo/blob/main/.env.example

Looking at https://github.com/fastify/demo/blob/main/package.json#L17 and following up with https://nodejs.org/en/learn/command-line/how-to-read-environment-variables-from-nodejs I just learned about the new (node v20) argument --env.

@onlywei
Copy link

onlywei commented Sep 15, 2024

@millette Thanks. I just copied the .env.example file and turned it into .env. Tried to run docker compose up again. The results were the same:

% docker compose up
[+] Running 1/0
 ✔ Container demo-db-1  Created                                                                                                                      0.0s 
Attaching to db-1
db-1  | 2024-09-15 01:22:02+00:00 [Note] [Entrypoint]: Entrypoint script for MySQL Server 8.4.2-1.el9 started.
db-1  | 2024-09-15 01:22:02+00:00 [Note] [Entrypoint]: Switching to dedicated user 'mysql'
db-1  | 2024-09-15 01:22:02+00:00 [Note] [Entrypoint]: Entrypoint script for MySQL Server 8.4.2-1.el9 started.
db-1  | 2024-09-15 01:22:02+00:00 [ERROR] [Entrypoint]: Database is uninitialized and password option is not specified
db-1  |     You need to specify one of the following as an environment variable:
db-1  |     - MYSQL_ROOT_PASSWORD
db-1  |     - MYSQL_ALLOW_EMPTY_PASSWORD
db-1  |     - MYSQL_RANDOM_ROOT_PASSWORD
db-1 exited with code 1

Next, I tried to add the following line into my .env file:

MYSQL_ALLOW_EMPTY_PASSWORD=true

That didn't work, the result was the same, so next I tried:

MYSQL_ALLOW_EMPTY_PASSWORD=1

Still the same result when running docker compose up.

I went ahead and tried to run npm run test anyway, and it did pick up the .env file but still errored as expected since my database never started:

% npm test         

> fastify-demo@0.0.0 test
> npm run db:seed && tap --jobs=1 test/**/*


> fastify-demo@0.0.0 db:seed
> node --env-file=.env scripts/seed-database.js

/Users/wei/github/demo/node_modules/mysql2/promise.js:253
  const createConnectionErr = new Error();
                              ^

Error
    at createConnection (/Users/wei/github/demo/node_modules/mysql2/promise.js:253:31)
    at seed (file:///Users/wei/github/demo/scripts/seed-database.js:4:28)
    at file:///Users/wei/github/demo/scripts/seed-database.js:60:1
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:123:5) {
  code: 'ECONNREFUSED',
  errno: undefined,
  sqlState: undefined
}

Node.js v20.15.0

@onlywei
Copy link

onlywei commented Sep 15, 2024

The variables MYSQL_USER and MYSQL_PASSWORD are referenced in the docker-compose.yml file, but it seems to be ignoring the .env file that I made.

src/client/mount.tsx Outdated Show resolved Hide resolved
export default {
base: '/',
root: resolve(import.meta.dirname, 'src/client'),
plugins: [viteReact()]
Copy link

Choose a reason for hiding this comment

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

Since you're using tsc to build the server code into the dist directory, I personally think it's kind of weird to have the client code built to src/client/dist. I prefer to add the following here to the vite config:

build: {
  emptyOutDir: true, // optional
  outDir: resolve(import.meta.dirname, 'dist/client'),
}

Additionally, I'm trying to get this PR into fastify-vite which gives another vite plugin called viteFastify() that helps with production builds.

@onlywei
Copy link

onlywei commented Sep 15, 2024

For docker, see https://docs.docker.com/compose/environment-variables/set-environment-variables/#use-the-env_file-attribute and https://docs.docker.com/compose/environment-variables/set-environment-variables/#set-environment-variables-with-docker-compose-run---env on the same page.

Thanks. Based on those docs here's what I tried:

First, I tried to run docker compose up --env -- results in "unknown flag: --env"
Next, I tried docker compose --env up -- results in "unknown flag: --env"

Finally, I tried adding env_file: .env to the docker-compose.yml file:

services:
  db:
    image: mysql:8.4
    env_file: .env <-- this is what I added
    environment:

After that, I ran docker compose up and still got the same error as before. For clarity, my .env file is an exact copy of the .env.example file.

@jean-michelet
Copy link
Contributor Author

Thanks for your review. I am gonna take a look to the issue tomorrow, I am off today.

@jean-michelet
Copy link
Contributor Author

@onlywei

Can you:

  • pull my changes
  • remove the volumes docker compose down -v
  • launch this command: docker compose up --build
  • re-run the migrations: npm run db:migrate
  • re-launch the test: npm run test

And tell me if it's ok now?

@jean-michelet
Copy link
Contributor Author

Also, I use Linux/Ubuntu and never used anything else since I learned programming so can you plz tell us if there specific Mac/Windows issues?

@onlywei
Copy link

onlywei commented Sep 16, 2024

@onlywei

Can you:

  • pull my changes
  • remove the volumes docker compose down -v
  • launch this command: docker compose up --build
  • re-run the migrations: npm run db:migrate
  • re-launch the test: npm run test

And tell me if it's ok now?

I pulled the changes. Things are indeed different now, but still not working on my Mac. Here are the observations on my MacBook:

docker compose up --build must not be run in detached mode -d or else npm run db:migrate fails to connect to the database.

npm run test results in the following message:

% npm run test

> fastify-demo@0.0.0 test
> npm run db:seed && tap --jobs=1 test/**/*


> fastify-demo@0.0.0 db:seed
> node --env-file=.env scripts/seed-database.js

All tables have been truncated successfully.
Users have been seeded successfully.
sh: tap: command not found

BTW I am using zsh which is the default on Macbooks, and my node version is 20.15.0

@onlywei
Copy link

onlywei commented Sep 17, 2024

This is quite strange. I see tap defined in the devDependencies of this project, yet I've done a number of repeated npm i commands and I can't find it within node_modules. I've even rm -rf node_modules package-lock.json and re-ran npm i, and tap just doesn't get installed.

@onlywei
Copy link

onlywei commented Sep 17, 2024

Ah, ok I figured it out that because the .env file had NODE_ENV=production set, the npm i command is affected. I am not used to that happening at all. The tests are running now!

There are 5 tests that are passing and 21 that are failing. Are those 21 the ones that you feel are related to fastify/vite? They seem to be complaining that the JWT_SECRET is missing. Was I supposed to do something to generate the JWT_SECRET first? @jean-michelet ?

@jean-michelet
Copy link
Contributor Author

Yes, you have to generate a secret and set to JWT_SECRET, this is just a hmac. It ensures this is your application that sign the token and tests depends on it. This is just a random set of letters and digits.

E.g. d7b3f2cfd0951e5b6c5e0bd0837af1ae951e8487b

@jean-michelet
Copy link
Contributor Author

Ah, ok I figured it out that because the .env file had NODE_ENV=production

This is expected, we don't use NODE_ENV to determine what feature to use or not.

@onlywei
Copy link

onlywei commented Sep 17, 2024

@jean-michelet OK now we're cooking. I generated a random JWT_SECRET using:

node -e "console.log(require('crypto').randomBytes(32).toString('hex'))"

And I put it into my .env file and now the tests run well. I humbly suggest adding these steps to the README.md file just in case anyone else is dumb like me and never did this before.

As for the tests themselves, they actually all pass even when I forcibly register vite:

image

Do I have to do something else to get them to fail?

Note that I also commented out the one assertion in rate-limit.test that says:

// assert.equal(res.body, "Vite is not registered.")

@jean-michelet
Copy link
Contributor Author

jean-michelet commented Sep 21, 2024

This is the CI that fail... In local it works fine.

Ok for the doc update.
I integrate your feedbacks today and ping you for a review.

@onlywei
Copy link

onlywei commented Sep 23, 2024

This is the CI that fail... In local it works fine.

Ok now I see the time outs in an earlier CI build.

Given that I'm not able to actually trigger the CI and test it myself, I can only offer some brute force suggestions.

My first suggestion would be that there should be a new release of @fastify/vite out soon that contains a big change. Specifically it will include a vite plugin that I recommend that all users also use in their vite.config.js. I would try that and see if it is still timing out in CI.

If that doesn't fix it, then the only thing I can think of to do next is to put console.log statements everywhere to see where it might be failing in CI.

Copy link

sonarcloud bot commented Nov 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarCloud

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.

4 participants