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

Add main frontend #342

Merged
merged 10 commits into from
Jun 22, 2021
Merged

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Jun 18, 2021

Main frontend for companion-docker

Setup is:

  • Vue 2
  • SASS (with dart-sass)
  • Typescript
  • PWA support
  • Router (with history mode)
  • Vuex
  • Vuetify

Adds test job for installing and building the frontend project.

This fixes #207, although there are import considerations there we should open new issue for in the future (regarding dynamic plugins).

Image available for test under rafaellehmkuhl/companion-core:frontend

core/services/bff/.gitignore Outdated Show resolved Hide resolved
core/services/bff/README.md Outdated Show resolved Hide resolved
core/services/bff/package.json Outdated Show resolved Hide resolved
core/services/bff/.editorconfig Outdated Show resolved Hide resolved
core/services/bff/package.json Outdated Show resolved Hide resolved
core/services/bff/src/registerServiceWorker.ts Outdated Show resolved Hide resolved
core/services/install-services.sh Outdated Show resolved Hide resolved
core/services/bff/tsconfig.json Outdated Show resolved Hide resolved
core/services/wifi/main.py Outdated Show resolved Hide resolved
core/services/bff/src/App.vue Outdated Show resolved Hide resolved
@rafaellehmkuhl rafaellehmkuhl changed the title [WIP] Introducing BFF [WIP] Introducing CompanionOS Jun 18, 2021
@rafaellehmkuhl rafaellehmkuhl force-pushed the introducing-bff branch 2 times, most recently from 2b6cfe1 to 36a75a3 Compare June 18, 2021 19:24
@rafaellehmkuhl rafaellehmkuhl changed the title [WIP] Introducing CompanionOS Introducing CompanionOS Jun 18, 2021
core/frontend/README.md Outdated Show resolved Hide resolved
@rafaellehmkuhl rafaellehmkuhl marked this pull request as draft June 18, 2021 20:27
@rafaellehmkuhl rafaellehmkuhl force-pushed the introducing-bff branch 4 times, most recently from 426d047 to cddf7bf Compare June 18, 2021 20:54
@rafaellehmkuhl rafaellehmkuhl changed the title Introducing CompanionOS Add main frontend Jun 18, 2021
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

The are some things that are conceptually wrong.
Files that are a size transformation should be created/generated by the build system, avoid unnecessary tracking and management of unnecessary files.
The patch is also missing a gitignore and dockerignore files, the first one is to ignore and help us as developers to avoid dealing with unnecessary noise under git diff, the second is important to avoid problems with docker cache system, check my comment for further explanation.
There is also PWA packages and some configuration files, but it's missing the configuration for service-worker.

I believe that is a good patch, but to get it merged we need to have at least a bare minimum correct structure and dependencies to avoid any pain for us in further development, once we hit the sweet spot of a good basic configuration for the typescript frontend we should be good to merge.

It's also failing in the CI.

core/Dockerfile Outdated Show resolved Hide resolved
core/frontend/.browserslistrc Outdated Show resolved Hide resolved
core/frontend/tsconfig.json Outdated Show resolved Hide resolved
core/frontend/src/main.ts Outdated Show resolved Hide resolved
@rafaellehmkuhl
Copy link
Member Author

The are some things that are conceptually wrong.
Files that are a size transformation should be created/generated by the build system, avoid unnecessary tracking and management of unnecessary files.

Please, check my comment on your first review. Those files are not just size transformation. There's background and margins added in the process. That's the only solution, unless we want to learn how to use and add a cli-image-editor-like tool on our build because of the tracking of 3 images that are all together.

The patch is also missing a gitignore and dockerignore files, the first one is to ignore and help us as developers to avoid dealing with unnecessary noise under git diff, the second is important to avoid problems with docker cache system, check my comment for further explanation.

You've previously suggested that we should not have gitignore for node_modules folder. Have you changed your mind on that?
About the dockerignore, I will add one.

There is also PWA packages and some configuration files, but it's missing the configuration for service-worker.

It's not, like I've answered on the first review. Vue automatically adds a default service worker during build time. We only need to specify one if we want custom behavior.

It's also failing in the CI.

Yes, this is failing since we removed the dedicated eslintrc file, like you suggested (which I found is reasonable), but it's needed on the frontend folder during build time. Do you have any suggestion on that?

@patrickelectric
Copy link
Member

suggested that we should not have gitignore for node_modules

Please link the commend, I said that dockerignore is missing and not that gitignore should not exist.

dedicated eslintrc file, like you suggested

As said, the file should not exist if you merge it with the project configuration. Check MGroundStation example

@rafaellehmkuhl
Copy link
Member Author

Please link the commend, I said that dockerignore is missing and not that gitignore should not exist.

That was outside github, but ok

As said, the file should not exist if you merge it with the project configuration. Check MGroundStation example

You're not understanding. That's not the case with MGS. That's a build problem from the fact that the dockerfile is on a sub-folder with relation to the eslintrc file, so there's no way to copy.

I've found a possible solution disabling the eslint on build, though.

@rafaellehmkuhl rafaellehmkuhl force-pushed the introducing-bff branch 5 times, most recently from 028f146 to 3378ef3 Compare June 18, 2021 22:24
Copy link
Member

@Williangalvani Williangalvani left a comment

Choose a reason for hiding this comment

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

Looking good so far.
27289a5 commit message is either missing "dockerfile" or has an extra colon

core/frontend/.gitignore Outdated Show resolved Hide resolved
.eslintrc.js Show resolved Hide resolved
@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Jun 21, 2021

The problem with the helper service is because it's fetching the backend through the latest path on the frontend, instead of the vX.Y path, like the other services.

image

For some strange reason, when this is done, the redirect URL is removing the /helper on the beggining of the URL, as can be seen in the image above.

As all other services are using v1.0 URLs for fetching, I moved this fetch on helper to v1.0 also and opened issue #344 to track this problem (inability to use /latestpaths on the frontends).

It was working before by a coincidence. It was the one that should not work, but the fact that it was on the root path (/) masked the problem.

@Williangalvani this seems to be related to nginx. Do you have any idea on what may be happening?

Setup is:
- Vue 2
- Typescript
- Vuetify
- Vuex
- Router (with history mode)
- PWA support
- SASS (with dart-sass)
- Font as dependency (for offline usage)
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@Williangalvani Williangalvani merged commit 8827743 into bluerobotics:master Jun 22, 2021
@rafaellehmkuhl rafaellehmkuhl deleted the introducing-bff branch June 22, 2021 12:49
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.

Core: Add general web application
3 participants