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

Change build to use Vite and Typescript #91

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

C-Loftus
Copy link

@C-Loftus C-Loftus commented Oct 30, 2024

This code does not add any new user-facing behavior and should hopefully not be too hard to review. I kept the same overall components and layout of the codebase since I think it is well thought out.

Before review, goal is to:

  • fix all existing eslint errors. i.e:
    • Get rid of v-html which can be insecure
    • Remove const self = this and any places where this is shadowed
    • Reduce global variables. Put all necessary global variables into a pinia store instead of mutating global this
    • Refactor functions which mutate parent props
  • switch off old dependencies to current vue standards (pinia instead of vuex etc)
    • I am only switching dependencies that have a recommended updated version. (i.e. vuex vs pinia). Otherwise I am keeping things the same.
    • Allows us to no longer need to use --force or --legacy-peer-deps
    • I am not adding any extra dependencies except ones that are a better supported version of an old one
  • switch to vite instead of webpack
  • update dockerfile
    • current dockerfile uses node 14 which is very old
  • switch to typescript instead of js
    • Realize that this is a significant change and if others feel differently, please feel free to let me know.
    • I have caught some current issues through this (mainly wrong types for vuetify like passing in a string vs number) and think it is worthwhile
  • add some tests and interfaces so we can feel more confident when doing some of the json

Addresses #90

@C-Loftus C-Loftus marked this pull request as draft October 30, 2024 21:29
@webb-ben webb-ben marked this pull request as ready for review November 11, 2024 20:20
@webb-ben
Copy link
Member

@C-Loftus. It appears there is something wrong with setting the environment variables in the Docker deployment. I am able to work around this by not setting any variables at root level:
image

Additionally, the UI displays a weird error when there are no observations published from a station.

@C-Loftus
Copy link
Author

Thanks for catching that @webb-ben
I will investigate that, fix it, then rerequest your review before others look.

@maaikelimper
Copy link
Collaborator

I deployed this version of the wis2box-ui on a test server and I get an error:
image

Test-server at http://136.156.130.194/

@C-Loftus C-Loftus marked this pull request as draft November 18, 2024 17:12
@C-Loftus C-Loftus marked this pull request as ready for review November 21, 2024 16:10
@C-Loftus
Copy link
Author

@webb-ben This should be fixed now if you want to review before the WMO folks. (But anyone feel free to take a look!)Here are some quick screenshots for context to show I fixed the error handling.

When used with the test server where there are no observations:
image

Where used with a real wis2box server i.e. the one for South Korea, but with a station with no observations
image

Otherwise everything should be ok and stable.

To replicate I did this

docker build .
lcp --proxyUrl "https://wis2box.kma.go.kr/" 
# or (lcp for http://136.156.130.194/ also works)
docker run -e WIS2BOX_API_URL=http://localhost:8010/proxy/oapi -p 5100:80 sha256:1d5697bd7ef4dd720469
56546a869ceba13a61f6ee476c4b52a12e1f8ab0cceb

@C-Loftus C-Loftus marked this pull request as draft November 21, 2024 17:50
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.

3 participants