Maintenance: Upgraded all packages. #630
Conversation
…on, and many imports that were remove
✅ Deploy Preview for lbgg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Things mostly look good, just curious about those few things I pointed out. Also I know why you mentioned you want to do explicit imports, but I do feel like having the vitest blocks being global is a pretty standard thing for most repos, and it feels kinda backwards here
| @@ -18,8 +18,7 @@ interface ForgotPasswordCardState { | |||
| } | |||
|
|
|||
| const emit = defineEmits<{ | |||
| (event: 'close'): void | |||
| (event: 'cancelClick'): void | |||
| (event: 'close' | 'cancelClick'): void | |||
There was a problem hiding this comment.
Would this imply that we only define one emitted event, as opposed to two like before? Or are TS and Nuxt smart enough to know to create two?
There was a problem hiding this comment.
So it was an eslint suggestion and i was suspicious myself, but it works. My guess is that it just reads it and sees the string passed it up
There was a problem hiding this comment.
Let's not prioritize the linter over whatever the commonly accepted pattern is. I feel like this could lead to issues when trying to use certain features of vue but I'm not sure. I suppose if it works and the tests all pass then it's all good.
| (event: 'close'): void | ||
| (event: 'forgotPasswordClick'): void | ||
| (event: 'signUpClick'): void | ||
| (event: 'close' | 'forgotPasswordClick' | 'signUpClick'): void |
| (event: 'close'): void | ||
| (event: 'logInClick'): void | ||
| (event: 'signUpClick'): void | ||
| (event: 'close' | 'logInClick' | 'signUpClick'): void |
| @@ -15,6 +15,7 @@ export async function useConfirmAccount( | |||
| baseUrl: useRuntimeConfig().public.backendBaseUrl, | |||
| }) | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-invalid-void-type | |||
There was a problem hiding this comment.
I feel like we can get around having to do this by updating the composable, but this is fine for now
There was a problem hiding this comment.
Agreed. Make an issue for it.
| @@ -54,6 +55,8 @@ describe('useApi', () => { | |||
| }) | |||
|
|
|||
| expect(onErrorSpy).toBeCalledTimes(1) | |||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
| // @ts-expect-error | |||
There was a problem hiding this comment.
Looking this instruction up, I'm not sure why we need this here. I don't see why the compiler would think the next line would be an error... 🤔
There was a problem hiding this comment.
Ya i'm not sure either, We can always go back and fix later as its not causing any errors it seems
| - name: Cache build | ||
| uses: actions/cache@v3 | ||
| id: cache-build | ||
| with: | ||
| path: .nuxt/ | ||
| key: ${{ runner.os }}-build-${{ hashFiles('**/.nuxt') }} | ||
| restore-keys: ${{ runner.os }}-build- |
There was a problem hiding this comment.
Is there a reason we're removing the caching step from the process?
There was a problem hiding this comment.
It didn't seem to work anymore, and not sure the benefit of caching builds specifically anyways as its good to have the build run in a roughly known good environment of CI
What
maintenance: upgraded all packages, many changes for ESLint 9 migration, and many imports that were remove
Acceptance
Steps for testing
things arent borked.