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 google analytics + fix ts compilation #925

Merged
merged 6 commits into from
Jan 22, 2024
Merged

add google analytics + fix ts compilation #925

merged 6 commits into from
Jan 22, 2024

Conversation

alee
Copy link
Member

@alee alee commented Dec 15, 2023

I don't think our CI is catching ts errors properly - yarn build generates a ton of complaints regarding pino if you run it manually

`yarn build` has been giving a lot of grief about proper types for the
pino logger
@alee alee requested a review from sgfost December 15, 2023 23:49
@sgfost
Copy link
Contributor

sgfost commented Dec 16, 2023

Building/type checking the server code doesn't seem to give me any errors or warnings on both 8a41e92 and 097eb0c. No clues whats happening there.. Is it preventing the build for you?

The client build problem looks like the same thing I ran into in #917 which seemed to get fixed by deleting the lockfile and node_modules and reinstalling but its probably bad news if the same thing is happening again

migration steps for future ref:
- replace yarn install with npm install
  - (npm ci == yarn install --frozen-lockfile)
- replace yarn <script> with npm run <script>
- use `--` when passing args to scripts called with npm run
- replace yarn.lock references with package-lock.json
- remove node_modules and yarn.lock, run npm i
- optionally prevent using yarn with package.json "engines" trick

resolves #926
* removed daisyui/tailwind packages
* removed vue-socket.io and peer deps

resolves #915
resolves #919
resolves #920
resolves #923
@sgfost
Copy link
Contributor

sgfost commented Jan 5, 2024

I'm guessing snyk is failing because its expecting to use yarn but I don't have access to it @alee

@alee
Copy link
Member Author

alee commented Jan 5, 2024

I'm guessing snyk is failing because its expecting to use yarn but I don't have access to it @alee

Just saw that - I think we can ignore this Snyk - I'll configure it to use a slightly better shared organizational account and send you an invite to that (i.e., not tied to my account 😅)

You're right about the TS errors disappearing on a clean rebuild which is just plain weird. I think it might be something non-deterministic related to our usage of docker volume mounts and node_modules and maybe a half-baked typescript upgrade that I was running locally. That said the errors I was seeing didn't seem wrong per se, mostly things like missing arguments to pino, or being oddly finicky about the type of an interpolated value in the log format string (i.e., %d on a string or something like that). I'll see if I can figure out how to reproduce it...

@sgfost
Copy link
Contributor

sgfost commented Jan 5, 2024

That is odd. I wonder how the parsing/type checking of format string args is even being done, didn't see anything glancing at pino's type definitions

@alee
Copy link
Member Author

alee commented Jan 5, 2024

one thing I'm not sure about is the logger.exception(e as Error) - I think anything can get thrown in JS so probably need to tighten up some of those invocations

https://medium.com/with-orus/the-5-commandments-of-clean-error-handling-in-typescript-93a9cbdf1af5

@sgfost
Copy link
Contributor

sgfost commented Jan 5, 2024

Yeah maybe unlikely to be an issue but should be easy to add

May be worth looking through some of the stricter @typescript-eslint rules at some point (no-throw-literal rule made me think of this). Enabling all of them with https://typescript-eslint.io/linting/configs/#strict-type-checked seems like way too much type gymnastics though

remove possibility of redundant
`docker compose -f base.yml -f staging.yml -f "staging.yml" ...`

that was causing errors in the latest version of docker compose v2.24.1
@alee alee merged commit 0630149 into main Jan 22, 2024
3 of 4 checks passed
@alee alee deleted the add_google_analytics branch January 31, 2024 03:17
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