13658 Vue cli to Vite migration #556
Conversation
JazzarKarim
force-pushed
the
13658-vue-cli-to-vite
branch
from
c477068
to
18a3afa
Compare
JazzarKarim
changed the title
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@babel/compat-data": "^7.21.5", | ||
| "@bcrs-shared-components/approval-type": "1.0.19", | ||
| "@bcrs-shared-components/base-address": "2.0.1", |
There was a problem hiding this comment.
BaseAddress was previously being used from "sbc-common-components". Now, the component is imported from the "bcrs-shared-components" repo. I've updated all the corresponding imports accordingly too in the code.
There was a problem hiding this comment.
Thank you for this, please take it for a spin as well
There was a problem hiding this comment.
I know there was a (test?) issue with using the shared BaseAddress component and Karim reverted that change, so it's now out of scope for this PR.
However, Travis, did you manage to use the shared BasedAddress component in an app? And unit test it? Or is there maybe something still wrong/needed with it?
package.json
Outdated
| "@bcrs-shared-components/breadcrumb": "2.1.15", | ||
| "@bcrs-shared-components/business-lookup": "1.1.17", | ||
| "@bcrs-shared-components/business-lookup": "1.1.29", |
There was a problem hiding this comment.
Updated this component as there's work there to fix an error corresponding to lodash.
There was a problem hiding this comment.
Would be nice to get rid of lodash eventually
There was a problem hiding this comment.
We could write the debounce function ourselves... in the future.
Please test that business-lookup works correctly, since it's been changed a bit since it was last used/tested here. Thx.
There was a problem hiding this comment.
BusinessLookup isn't working correctly. I don't think it has anything to do with Vite. Maybe recent changes in the shared repo broke it?
There was a problem hiding this comment.
I'm not getting any results back:
There was a problem hiding this comment.
The new keys have a typo.
There was a problem hiding this comment.
Still not working in the temporary URL. Let's leave it for now and I'll check if it's working when I deploy the branch to DEV. Is that fine?
There was a problem hiding this comment.
The API key in the API call request headers is still empty.
Yes, leave it for now.
There was a problem hiding this comment.
Is it still planned to temporary deploy this to Dev and test it there?
There was a problem hiding this comment.
Yes Sev. Currently, I see the Content Security Policy error. It's this and the address complete (they both work fine in local).
| "vuetify": "^2.6.15", | ||
| "vuex": "^3.6.2" | ||
| }, | ||
| "devDependencies": { | ||
| "@originjs/vite-plugin-require-context": "1.0.9", |
There was a problem hiding this comment.
There is require.context in CompleteResolution.vue. This plugin is to fix that (enables require.context with Vite).
src/mixins/document-mixin.ts
Outdated
| } | ||
|
|
||
| window.pdfjsWorker = pdfjsWorker | ||
| pdfjs.GlobalWorkerOptions.workerSrc = `https://cdnjs.cloudflare.com/ajax/libs/pdf.js/${pdfjs.version}/pdf.worker.js` |
There was a problem hiding this comment.
I've spent hours on trying to get the pdfjs library to work with Vite. This is literally the only way that I got it to work. Unit tests pass.
Here's a link if you want to try it: https://business-create-dev--pr-556-nilgmamu.web.app/incorporation-memorandum?id=TA7zV53Hu5
There was a problem hiding this comment.
I had a few links for this - one sec, have you tried them?
https://github.com/originjs/vite-plugins/tree/main/packages/vite-plugin-commonjs
There was a problem hiding this comment.
The second link is exactly what I used to get it to work 😆
There was a problem hiding this comment.
Rofl sorry HAHA i just looked and saw. I think there's a way we could include the library in our build package and refer to that instead of CDN.
There was a problem hiding this comment.
const pdfjsWorker = await import('pdfjs-dist/build/pdf.worker.entry');
pdfjs.GlobalWorkerOptions.workerSrc = pdfjsWorker;
Maybe possible to download the CDN and include it that way? Haven't tried, relying on CDN sucks
https://stackoverflow.com/questions/73979621/how-do-i-use-pdfjs-with-vue3-and-vite
There was a problem hiding this comment.
Travis, you're incredible. Something of the sorts works. Now, I don't have to add the pdfjs library to the CSP anymore which is what I would've preferred! Thank you so much Travis!
There was a problem hiding this comment.
Travis, please check latest commit. I managed to get pdfjs to work without CDN and with the unit tests to run also.
There was a problem hiding this comment.
If we do need an external JS library (at runtime) then I recommend we cache it locally (as we've done with address-complete). No CDN.
There was a problem hiding this comment.
Agreed. Makes sense.
| @@ -15,15 +15,14 @@ import { CorpTypeCd, FilingTypes } from '@/enums' | |||
| import { CourtOrderStepIF, DefineCompanyIF, EffectiveDateTimeIF, IncorporationAgreementIF, NameRequestIF, | |||
| OrgPersonIF, PeopleAndRoleIF, ShareStructureIF, TombstoneIF } from '@/interfaces' | |||
| import { ShareClassIF } from '@bcrs-shared-components/interfaces' | |||
|
|
|||
| Vue.use(Vuetify) | |||
| import { vi } from 'vitest' | |||
There was a problem hiding this comment.
Jest is replaced by vi in Vite. The methods work similar and have similar names as below.
There was a problem hiding this comment.
Is this the same as vitest ?
There was a problem hiding this comment.
Hmm, I've actually haven't stumbled upon the vitest.
I've seen that vi is the one that's being widely used: https://vitest.dev/api/vi.html
I'm guessing they're the same.
There was a problem hiding this comment.
OK, I have no further comments on this.
| export default defineConfig(() => { | ||
| return { | ||
| define: { | ||
| 'import.meta.env.ABOUT_TEXT': |
There was a problem hiding this comment.
This was in vue.config.js. It is now here and instead of importing it using process.env.ABOUT_TEXT, we use now import.meta.env.ABOUT_TEXT.
| : aboutText2 ? `"${aboutText2}"` | ||
| : '' | ||
| }, | ||
| envPrefix: 'VUE_APP_', // Need to remove this after fixing vaults. Use import.meta.env with VUE_APP. |
There was a problem hiding this comment.
Usually, if you want to import.meta.env, the environment variables must be prefixed with VITE_. This overrides that and makes import.meta.env work with VUE_APP_ as before.
The reason why I didn't rename environment variables to have a prefix of VITE_ is because of the ops code in one of the directories. That's how we usually populate our environment variables like the Genesys web message for example (from 1Password).
There was a problem hiding this comment.
Did you try renaming the env variables in vaults.env? If that didn't work then we'll need @pwei1018 to help with this (when he returns from vacation).
There was a problem hiding this comment.
Sev, can I please see what the variables look like in 1Password? You showed me the vaults once but I can't remember. Did they have the keys prefixed with VUE_APP?
There was a problem hiding this comment.
The variables in 1Password do not start with VUE_APP. That's a local renaming of the env keys. The variables in 1P are the last token on each line:
Eg, AUTH_API_URL
There was a problem hiding this comment.
I don't know if you can simply rename the env keys. I'm guessing there's a GCP step that uses vaults.env to look up the keys, assigns them to env keys, and then bakes them into the app js bundle.
I think that .env is our local way of having the keys without going through GCP.
There was a problem hiding this comment.
Sev, I've tried to change the environment variables prefix to VITE_ and test in the temporary URL. It doesn't work 😟
Check this link: https://business-create-dev--pr-556-nilgmamu.web.app/registration-define-business?id=T9SwNW7ISG
The web messaging doesn't appear. Looks like the environment variable wasn't populated. I'm guessing there is some configuration needed to be done like you mentioned. I'll be reverting this change.
There was a problem hiding this comment.
And I guess you're not merging this until we figure it out. @seeker25 ?
There was a problem hiding this comment.
I think it should be fine for the time being Sev. When I have VUE_APP as the prefix, all environment variables get populated correctly. I can see the web messaging etc..
It's just the address complete variable that's not getting populated. Also, I'm merging now not to main, but to a new branch 17118-vite-test-branch
Please let me know if that's OK with you Sev.
There was a problem hiding this comment.
Right, you have that config to override the prefix.
Ok to try merging to a branch and deploying it to Dev! Maybe post a message to let other devs know.
| @@ -17,6 +17,6 @@ | |||
| </strong> | |||
| </noscript> | |||
| <div id="app"></div> | |||
| <!-- built files will be auto injected --> | |||
| <script type="module" src="/src/main.ts"></script> | |||
There was a problem hiding this comment.
In Vite, the index.html is moved to the root directory and is changed accordingly. No more <%= BASE_URL %>.
| "@bcrs-shared-components/limited-restoration-panel": "1.0.5", | ||
| "@bcrs-shared-components/mixins": "1.1.10", | ||
| "@bcrs-shared-components/mixins": "1.1.20", |
There was a problem hiding this comment.
I've also updated a bunch of the useful packages as above like the enums, interfaces, and mixins.
There was a problem hiding this comment.
Yes, those are generally safe to update since we usually only add things to them.
|
/gcbrun |
|
Temporary Url for review: https://business-create-dev--pr-556-nilgmamu.web.app |
|
@severinbeauvais 3 tests (lines) that I can't seem to fix:
Shall I spend more time on trying to fix them? Please Sev I'd like your advice and thank you! |
The test is looking for the SbcFeeSummary but not finding it. Does the actual code display it in locally or temp URL?
It's odd that mockUpdateFiling was called but hasn't returned. Since the mock returns a promise, does the mock have to be awaited (not in the test but in the code)?
Any idea why CP voluntary dissolution fails? Due to PDF stuff maybe?
Let me have a look at these. Make sure you have tested as many flows as possible through Create UI (maybe keep a list of them for Riyaz to see). And then let's get this PR merged asap. @ketaki-deodhar @leodube-aot Please make time asap to go over this so you are aware of the changes (and in case one of you makes the same changes to Filings UI or Edit UI). Thanks. |
|
At least one of the unit test issues is because I think you can change the computed to: A better option would be to mock the feature flags so that we don't use this check in production code, but we can do that in a future ticket (out of scope for now). |
…port to shared components
|
@severinbeauvais Sev, I'm taking a look and going through the code based off your last two comments. Thank you so much. I'm looking into them right now. |
|
All unit tests have been fixed now. |
|
/gcbrun |
|
Temporary Url for review: https://business-create-dev--pr-556-nilgmamu.web.app |
* Vue cli to Vite migration (incomplete) * Migrated Vue CLI to Vite + all necessary changes * fix AddressComplete not initialized due to missing script and/or key * another try to fix addresscomplete * another try to fix addresscomplete * testing addresscomplete * another test for addresscomplete * revert addresscomplete change * removed useless dependency * added blob to content security policy * changed CSP * Another attempt to fix CSP * Fixed pdfjs library + unit tests + removed pdfjs script * small removals of useless stuff * updated pdfjs library * attempting with jsdom * going back to happy-dom after fixing tests with threads * Fixed icon + small cleanup in response to comments * Trying out VITE_ prefix * Reverting VITE_ to VUE_APP_ * restoring regenerator-runtime dependency * Removed services package and returned core-js package * fixed environment prefix issue * updated business lookup * added missing keys * Fixed typo * Fixed occasional issue arising from baseaddress after changing the import to shared components * Fixed App and CompleteResolution unit tests * Fixed some comments after moving from Jest to Vitest * More jest to vitest fixes * Fixed Actions unit test + little cleanup * Changed VITEST_WORKER_ID to VITEST Co-authored-by: JazzarKarim <122301442+JazzarKarim@users.noreply.github.com>
severinbeauvais
changed the title
|
Yesterday, we merged this PR into a feature branch in bcgov with the intent to temporarily deploy the branch to Dev to test the environment keys. However, the CD action failed (skipped) because the feature branch did not follow the correct naming convention. So we renamed the feature branch... but the CD action still failed. So I created another feature branch (correctly named)... but the CD action still failed. I renamed the original feature branch to its original name (to avoid confusion). The new feature branch is named "feature-vite-test-branch-2". @pwei1018 Can we get your help on this, please? |
* Vue cli to Vite migration (incomplete) * Migrated Vue CLI to Vite + all necessary changes * fix AddressComplete not initialized due to missing script and/or key * another try to fix addresscomplete * another try to fix addresscomplete * testing addresscomplete * another test for addresscomplete * revert addresscomplete change * removed useless dependency * added blob to content security policy * changed CSP * Another attempt to fix CSP * Fixed pdfjs library + unit tests + removed pdfjs script * small removals of useless stuff * updated pdfjs library * attempting with jsdom * going back to happy-dom after fixing tests with threads * Fixed icon + small cleanup in response to comments * Trying out VITE_ prefix * Reverting VITE_ to VUE_APP_ * restoring regenerator-runtime dependency * Removed services package and returned core-js package * fixed environment prefix issue * updated business lookup * added missing keys * Fixed typo * Fixed occasional issue arising from baseaddress after changing the import to shared components * Fixed App and CompleteResolution unit tests * Fixed some comments after moving from Jest to Vitest * More jest to vitest fixes * Fixed Actions unit test + little cleanup * Changed VITEST_WORKER_ID to VITEST
Issue #: /bcgov/entity#17118
Description of changes:
Please see bcgov/entity#13658 (comment) and the comments below for all the details.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).