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

Migrate to vue3 #1034

Merged
merged 18 commits into from Mar 28, 2023
Merged

Migrate to vue3 #1034

merged 18 commits into from Mar 28, 2023

Conversation

csc-felipe
Copy link
Contributor

@csc-felipe csc-felipe commented Mar 16, 2023

Description

Migrate vue2 → vue3.

Most of the breaking changes don't apply to our code, so it's not such a big code change. But still quite a large PR.

https://v3-migration.vuejs.org/breaking-changes/#template-directives
https://v3-migration.vuejs.org/breaking-changes/v-model.html
https://vuejs.org/guide/reusability/custom-directives.html#directive-hooks

Related issues

GL 943

needs #982 #1006

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Changes Made

  • Update libraries
    • vue
    • vuex
    • vue-i18n
    • vue-router
    • fix changing project
  • refactor index page app creation to be less repetitive
  • update csc-ui vue directive to work with vue3
  • add IndexedDB workaround for Chrome - Random Closing Database issue (hard/impossible to reproduce) dexie/Dexie.js#613
  • remove LAST_ACTIVE cookie reference, as it's not being set anywhere
  • fix bindings in autocomplete fields by using :query instead of v-model
  • some values that were not reactive, now are, and can't be passed as is to Dexie, they need toRaw() from vue

Testing

  • Tests do not apply
  • Manual tests are in order to confirm everything works as before

Mentions

In VSCode, the Vue extension change from Vetur to Volar. So you might want to update your setup.

Also, remember to pnpm install to update the libraries.

Vue development browser extension now works

@csc-felipe csc-felipe changed the base branch from devel to feature/vite March 16, 2023 10:19
@csc-felipe csc-felipe force-pushed the feature/vue3 branch 2 times, most recently from 66b7c93 to b90f207 Compare March 20, 2023 08:22
@csc-felipe csc-felipe self-assigned this Mar 20, 2023
@csc-felipe csc-felipe force-pushed the feature/vue3 branch 2 times, most recently from ebcccd5 to bc66159 Compare March 20, 2023 08:29
@csc-felipe csc-felipe changed the title WIP: Migrate to vue3 Migrate to vue3 Mar 20, 2023
@csc-felipe csc-felipe requested a review from hannyle March 20, 2023 08:32
@csc-felipe
Copy link
Contributor Author

Ready for review

Base automatically changed from feature/vite to devel March 20, 2023 08:36
@csc-felipe
Copy link
Contributor Author

rebased

@csc-felipe
Copy link
Contributor Author

There was a weird issue with indexedDB, in which the db transactions were not completing. So there's a workaround. According to issues I've been reading in the dexie repo, it's mostly a chromium issue, but It only happened after migrating to vue, which makes me wonder if there's a connection between these.

Apparently, router.go changed behaviour, and selecting a project was not working, so that was fixed as well.

I noticed that the LAST_ACTIVE cookie was not being set anywhere, and remove the code referencing it.

@hannyle
Copy link
Contributor

hannyle commented Mar 21, 2023

I'm getting these warnings

Screenshot 2023-03-21 at 14 19 21

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 21, 2023

I see the warnings too. The vue-router warning is what happens during component initialization. The other warnings are from i18n vite plugin. It's not recommended to have HTML in the strings, and it was a deliberate choice, as I didn't know at the time (and still don't) how else to accomplish the same thing.that.

@hannyle
Copy link
Contributor

hannyle commented Mar 21, 2023

I see the warnings too. The vue-router warning is what happens during component initialization. The other warnings are from i18n vite plugin. It's not recommended to have HTML in the strings, and it was a deliberate choice, as I didn't know at the time (and still don't) how else to accomplish the same thing.that.

I could also help take a look if I can fix this, if you don't mind.

@hannyle hannyle mentioned this pull request Mar 21, 2023
1 task
@hannyle
Copy link
Contributor

hannyle commented Mar 22, 2023

After the fix for autocomplete, the previous issue has gone but I think in the Upload modal, when you type some letters it doesn't suggest the result anymore. It still works fine for Searchbox I think.

@csc-felipe
Copy link
Contributor Author

After the fix for autocomplete, the previous issue has gone but I think in the Upload modal, when you type some letters it doesn't suggest the result anymore. It still works fine for Searchbox I think.

It should be fixed now

@csc-felipe csc-felipe changed the title Migrate to vue3 WIP: Migrate to vue3 Mar 22, 2023
@csc-felipe csc-felipe changed the title WIP: Migrate to vue3 Migrate to vue3 Mar 23, 2023
Copy link
Member

@blankdots blankdots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my point of view stuff works, and I cannot spot anything in the code.

All i can spot in the build are these warnings:

emcc: warning: warnings in JS library compilation [-Wjs-compiler]
emcc: warning: running limited binaryen optimizations because DWARF info requested (or indirectly required) [-Wlimited-postlink-optimizations]
vite v4.2.0 building for production...
✓ 490 modules transformed.
rendering chunks (60)...warnings when minifying css:
▲ [WARNING] Unexpected ";" [css-syntax-error]

    <stdin>:10223:1:
      10223 │ };
            ╵  ^

@blankdots
Copy link
Member

blankdots
blankdots previously approved these changes Mar 24, 2023
Copy link
Member

@blankdots blankdots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from my point of view it works, but please wait for other reviewers

@hannyle
Copy link
Contributor

hannyle commented Mar 24, 2023

I have this issue when using the Edit tags modal, the tags were not properly saved and might be a lingering issue of dexie :/

Screenshot 2023-03-24 at 12 01 51

Copy link
Member

@sampsapenna sampsapenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm experiencing a bug, where after login the navbar is partially empty because the username wasn't in the URL from the beginning, maybe it's breaking reactivity? After refreshing everything is properly rendered, but if I manually navigate to /browse again, the bug is repeated.

It might have something to do with router being more strict about the router parameters, complaining that the initial /browse doesn't have those defined while some component tries accessing them.

Screenshot from 2023-03-24 14-29-37

Screenshot from 2023-03-24 14-30-46

@hannyle
Copy link
Contributor

hannyle commented Mar 24, 2023

I noticed that bug as well, I think @csc-felipe created a separate issue for it?
https://gitlab.ci.csc.fi/sds-dev/sd-connect/swift-browser-ui/-/issues/972

@sampsapenna
Copy link
Member

I noticed that bug as well, I think @csc-felipe created a separate issue for it? https://gitlab.ci.csc.fi/sds-dev/sd-connect/swift-browser-ui/-/issues/972

Seems like it, thanks for the info 👍🏼

sampsapenna
sampsapenna previously approved these changes Mar 24, 2023
@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 27, 2023

I have this issue when using the Edit tags modal, the tags were not properly saved and might be a lingering issue of dexie :/

These issues are because of the reactivity model in Vue3, and it wraps everything with reactive(). Calling isProxy() confirms it.

The trick is to use toRaw() before passing a value to Dexie.

@csc-felipe
Copy link
Contributor Author

Makes me think that we may not even need the workaround... But well, it's a good workaround anyway

@csc-felipe
Copy link
Contributor Author

What it means is that the array with tags, for example, becomes a ProxyArray, actually an object. So Dexie doesn't like it ^^.

@hannyle
Copy link
Contributor

hannyle commented Mar 27, 2023

I wonder why it happens only with Edit tags but not with creating folder or copy folder? Or maybe Share Modal when we change the list of Shared IDs ?

@csc-felipe
Copy link
Contributor Author

It happens only when the Proxy object is passed to Dexie, and Dexie tries to do something with it. And I exagerated that everything becomes a reactive proxy object. This same case probably doesn't apply to the previous issue.

Vue wraps the tags array into a proxy, and
dexie was crashing. Using toRaw(this.tags)
returns the actual array.
@csc-felipe csc-felipe dismissed stale reviews from sampsapenna and blankdots via 7202ef5 March 27, 2023 12:08
@csc-felipe
Copy link
Contributor Author

That's hopefully fixed now!

@hannyle
Copy link
Contributor

hannyle commented Mar 28, 2023

That's hopefully fixed now!

Yes it's fixed 👍

@hannyle
Copy link
Contributor

hannyle commented Mar 28, 2023

There is a similar issue with Display Options in ObjectsView that changing 3 options shows the same error with Dexie.

I did a quick research and understand that because Vue 3 has built-in reactivity , we could also use liveQuery() to have reactive data-fetching or query, for example:

Screenshot 2023-03-28 at 9 03 30

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 28, 2023

I'm not sure I understand the suggestion to use liveQuery, because it's usually for reading data, but that db call is writing data.

@hannyle
Copy link
Contributor

hannyle commented Mar 28, 2023

Well that's true, hm.. 🤔

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 28, 2023

toRaw also works here, so we don't have to pass the reactive values to dexie.

It should be fixed now.

Copy link
Contributor

@hannyle hannyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Great job! 🎉

@csc-felipe csc-felipe merged commit f968482 into devel Mar 28, 2023
14 checks passed
@csc-felipe csc-felipe deleted the feature/vue3 branch March 28, 2023 07:28
blankdots pushed a commit that referenced this pull request Jun 13, 2023
Bugfix/fix search issues

Closes #1034

See merge request sds-dev/sd-connect/swift-browser-ui!55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Technical debt enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants