Package updates + lots of cleanup #503
Conversation
JazzarKarim
force-pushed
the
16207-package-updates-and-cleanup
branch
from
7411626
to
aff0769
Compare
| sessionStorage.setItem('TEMP_REG_NUMBER', 'TaAbBcC123') | ||
| businessStore.setLegalType(CorpTypeCd.BENEFIT_COMPANY) | ||
| }) | ||
| // beforeAll(() => { |
There was a problem hiding this comment.
This is done to fix the Invalid: beforeAll() may not be used in a describe block containing no tests eslint error.
| @@ -335,7 +327,7 @@ export default class StaffNotation extends Vue { | |||
|
|
|||
| @Emit('close') | |||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |||
| private close (needReload: boolean): void {} | |||
| close (needReload: boolean): void { /* no empty function */ } | |||
There was a problem hiding this comment.
There are a lot of these in the code. This is done to fix the Unexpected empty method SonarCloud code smell.
For reference: https://eslint.org/docs/latest/rules/no-empty-function
| @Prop({ default: null }) readonly highlightId!: number | ||
|
|
||
| @Getter(useFilingHistoryListStore) getCurrentFiling!: ApiFilingIF | ||
| @Getter(useConfigurationStore) getEditUrl!: string | ||
| @Getter(useFilingHistoryListStore) getFilings!: Array<ApiFilingIF> | ||
| @Getter(useBusinessStore) getIdentifier!: string |
There was a problem hiding this comment.
Eslint was complaining that this getter was also in the mixin so I removed it. This also applies to many places in the code.
There was a problem hiding this comment.
That's fine. I saw this (and changed it) too.
There was a problem hiding this comment.
(It's because the mixins are now properly "mixed into" this, so their method signatures are now recognized.)
There was a problem hiding this comment.
I'm OK to resolve this conversation.
| @Getter(useBusinessStore) getIdentifier!: string | ||
| @Getter(useRootStore) getNameRequest!: any | ||
| @Getter(useConfigurationStore) getPayApiUrl!: string | ||
| @Getter(useFilingHistoryListStore) getPendingCoa!: ApiFilingIF | ||
| @Getter(useRootStore) getTasks!: Array<ApiTaskIF> | ||
| @Getter(useRootStore) getTodoListResource!: TodoListResourceIF | ||
| @Getter(useBusinessStore) isBenBcCccUlc!: boolean | ||
| @Getter(useBusinessStore) isGoodStanding!: boolean | ||
| @Getter(useBusinessStore) isSoleProp!: boolean |
There was a problem hiding this comment.
This getter was being used in the code but it wasn't here 😅
| // this is needed to remove existing service workers on users' systems | ||
| // ref: https://www.npmjs.com/package/webpack-remove-serviceworker-plugin | ||
| // ref: https://github.com/NekR/self-destroying-sw/tree/master/packages/webpack-remove-serviceworker-plugin | ||
| new RemoveServiceWorkerPlugin({ filename: 'service-worker.js' }), |
There was a problem hiding this comment.
Reason for removing this: bcgov/entity#16170
There was a problem hiding this comment.
You may be able to delete src/registerServiceWorker.ts.
See #266 for the changes that were done to implement this feature that you are now removing.
There was a problem hiding this comment.
If the service worker is stuck - There's a certain header you can setup on NGINX that gets it unstuck. We had to do this when we loaded on a bad version of the service worker that turfed some clients. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Clear-Site-Data
There was a problem hiding this comment.
That's nice to know. Instead, we used this plugin for over a year to remove the SW.
There was a problem hiding this comment.
Yeah the plugin wouldn't work if the service worker is toast and errors in javascript - Kinda last ditch effort we had to do with Director Search
There was a problem hiding this comment.
Sev and Travis, I removed the registerServiceWorker.ts file with it's respective import in main.ts in my latest commit. Please let me know if you think that's OK to do.
There was a problem hiding this comment.
I think so... ? Try npm run build. Maybe we won't know for sure until it's deployed to Dev.
There was a problem hiding this comment.
I did try a build, everything looks OK. I also tested in local. Everything seems to be running normally. Okay then, hopefully all goes smoothly when I merge. Thanks!
| @@ -3,6 +3,8 @@ | |||
| "target": "esnext", | |||
| "module": "esnext", | |||
| "strict": false, | |||
| "noImplicitAny": false, | |||
| "strictNullChecks": false, | |||
There was a problem hiding this comment.
Added the two rules to stay consistent between all the 3 UIs.
There was a problem hiding this comment.
Might want to add in https://www.typescriptlang.org/tsconfig/skipLibCheck.html
What do you think Severin?
There was a problem hiding this comment.
Do we skip type checking of declaration files anywhere else? I'd rather my build take a few extra seconds than give up type-checking :)
There was a problem hiding this comment.
Maybe not such a good idea considering bcrs-shared-components isn't precompiled
| allDirectors: DirectorIF[] = [] | ||
|
|
||
| /** Initialize dir to use in the template's for loop. */ | ||
| dir: DirectorIF = { |
There was a problem hiding this comment.
Some tests were failing in Directors.spec.ts. The reasons being:
- Property or method "dir" is not defined on the instance but referenced during render.
- TypeError: Cannot read properties of null (reading 'id'). This also is related to dir.
This change fixes the tests.
There was a problem hiding this comment.
This is a decoy fix, which calms the tests perhaps, but doesn't solve the problem.
I see on line 191 that dir is a loop variable. Maybe try renaming it in case it's a reserved word? You could also try moving the v-for to a template element around the <li>.
There was a problem hiding this comment.
Both didn't work Sev. I was thinking of adding optional chaining to dir in the template. Apparently, it's not supported in Vue 2. It is in Vue 3: vuejs/vue#11088
I'm thinking of disabling the tests for the time being and adding them to the new ticket. I'll look into this more and try to fix before doing so.
There was a problem hiding this comment.
Does the code work correctly right now?
If it does then create a follow-up ticket with the comments above and move on.
There was a problem hiding this comment.
I've been messing around with the Directors and I see everything is working correctly with no errors. So, I guess the code is OK.
Alright, got it.
| import { ContactInfo } from '@/components/common' | ||
| import { HistoryItemIF } from '@/interfaces' |
There was a problem hiding this comment.
The interface HistoryItemIF doesn't exist.
Codecov Report
@@ Coverage Diff @@
## main #503 +/- ##
==========================================
+ Coverage 82.88% 91.15% +8.26%
==========================================
Files 109 169 +60
Lines 2104 2747 +643
Branches 659 301 -358
==========================================
+ Hits 1744 2504 +760
+ Misses 360 239 -121
- Partials 0 4 +4
|
src/components/Dashboard/FilingHistoryList/filings/LimitedRestoration.vue
Outdated
Show resolved
Hide resolved
JazzarKarim
requested review from
severinbeauvais,
ketaki-deodhar and
chenhongjing
src/components/Dashboard/FilingHistoryList/LimitedRestorationExtensionFiling.vue
Outdated
Show resolved
Hide resolved
src/components/Dashboard/FilingHistoryList/filings/IncorporationApplication.vue
Show resolved
Hide resolved
JazzarKarim
force-pushed
the
16207-package-updates-and-cleanup
branch
from
f591c08
to
9f4f559
Compare
There was a problem hiding this comment.
I did go through all the changes. Lots of learning from the conversations. Great work @JazzarKarim ! 👍
|
Kudos, SonarCloud Quality Gate passed!
|
|
/gcbrun |
|
Temporary Url for review: https://business-filings-dev--pr-503-8u5hy650.web.app |
Issue #: /bcgov/entity#16207
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).