converting more components to setup composition api #1892
Conversation
ferferga
force-pushed
the
vite
branch
2 times, most recently
from
7a71b7b
to
f05baf9
Compare
ferferga
force-pushed
the
composition_api
branch
from
7384592
to
73e1de6
Compare
There was a problem hiding this comment.
First round of feedback, will carefully review the files later:
The layout at logs page is a bit borked (in master, the arrow is at the left):
I don't remember why we had the SelectedDeviceInfo component in the first place, the info it provides is the same that can be located at the table. @ThibaultNocchi do you remember something?
I think we can get off without it. In case we go for it again, the table should have a hover effect, so it's obvious for the user that there's an action on click.
The data returned by useDataFns has double quotes. This is because it's a ref, and it's handled upstream at vuejs/core#7306. For now, I would say just unwrap .value there.
In any case, those functions could be moved out of the template imo.
The check for the admin pages lives at plugins/router/middlewares/admin-pages.ts. If you look at the check, the redirection is made when the user is an admin, which should be changed to when the user IS NOT an admin.
aweebs
force-pushed
the
composition_api
branch
2 times, most recently
from
7f1cebc
to
eebd933
Compare
|
@ferferga this is ready for a proper review, I've tested the changes locally fairly well to confirm things appear correct. |
|
@ferferga I don't remember why too, so I guess it can be left off. |
There was a problem hiding this comment.
Change useDataFns to return direct value rather than ref for now
When I started the migration to Vue 3, I approached this problem like you did here. However, this is not correct because of the following reasons (in fact the useDateFns has been recently modified to always return refs):
- I recall returning values directly broke reactivity in some situations, so it was inconsistent. Reactive effects (Ref or ComputedRef) always ensured a consistent experience in that regard.
- Because you're returning a value, I believe the effects are not disposed properly when the component is unmounted, which could leak to memory leaks.
- You don't really know if something is reactive or not when using primitives, but you do when using effects/refs. DX-wise this is much better when you know when stuff is reactive or not.
I know it's painful, but hopefully this gets fixed out soon. I'm already tracking that issue because I myself reported it in the Vue discord and fixing this in the future it's a matter of making a PR once the fix is upstream to replace .value }} with }}
aweebs
requested review from
ThibaultNocchi and
ferferga
and removed request for
ThibaultNocchi
aweebs
force-pushed
the
composition_api
branch
2 times, most recently
from
3cf6b22
to
8524b5a
Compare
Also removes the old data table implementation.
…to setup composition API
|
Kudos, SonarCloud Quality Gate passed!
|
Cloudflare Pages deployment
|
Converts the remaining components in the
vitebranch to setup composition api.Replaces v-data-table usage in apikeys and devices with regular tables, since the data-table component is not available yet but also is likely overkill for this purpose. Let me know if you'd like these reverted.
Recommend reviewing commit by commit and hiding whitespace changes.
TODO
useDataFnsto return direct value rather than ref for now because of fix(shared): unwrap refs in toDisplayString vuejs/core#7306Follow up work (for future PRs)
UpNextbutton again