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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Vue3: Update vue-router #8215

Merged
merged 18 commits into from
Jan 17, 2023
Merged

[full-ci] Vue3: Update vue-router #8215

merged 18 commits into from
Jan 17, 2023

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Jan 11, 2023 •
edited
Loading

Description

Update vue-router to 4.x which is compatible with Vue 3.

Related Issue

  • Fixes <issue_link>

Motivation and Context

Fix warnings and allow switching to Vue 3 without compat mode at some point.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Make it fully work 🤷🏻
  • Fix patchRouter for clean urls

@update-docs
Copy link

update-docs bot commented Jan 11, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@delete-merged-branch delete-merged-branch bot deleted the branch master January 12, 2023 09:37
@dschmidt dschmidt force-pushed the vue3-vue-router branch 2 times, most recently from 13cb571 to dafa087 Compare January 14, 2023 17:13
@dschmidt dschmidt changed the title Vue3: Update vue-router [full-ci] Vue3: Update vue-router Jan 14, 2023
@dschmidt dschmidt changed the base branch from vue3-portal-vue to master January 14, 2023 17:14
@ownclouders
Copy link
Contributor

ownclouders commented Jan 14, 2023 •
edited
Loading

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/31616/13/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUILogin-adminBlocksUser_feature-L20.png

webUILogin-adminBlocksUser_feature-L20.png

webUILogin-oauthLogin_feature-L41.png

webUILogin-oauthLogin_feature-L41.png

webUILogin-oauthLogin_feature-L49.png

webUILogin-oauthLogin_feature-L49.png

@ownclouders
Copy link
Contributor

ownclouders commented Jan 14, 2023 •
edited
Loading

Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/31658/12/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-alice-2023-1-16-11-41-38.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-anonymous-2023-1-16-11-42-11.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-brian-2023-1-16-11-41-47.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-carol-2023-1-16-11-41-59.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-marie-2023-1-16-11-41-51.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/31658/tracing/participant-management-richard-2023-1-16-11-41-55.zip

@dschmidt dschmidt requested review from fschade and kulmann January 15, 2023 23:02
@dschmidt dschmidt marked this pull request as ready for review January 16, 2023 08:56
kulmann
kulmann reviewed Jan 16, 2023
View reviewed changes
packages/web-runtime/src/router/patchCleanPath.ts
@@ -7,25 +7,32 @@ import get from 'lodash-es/get'
// should immediately go away and be removed after finalizing the update
// to apply the patch to a route add meta.patchCleanPath = true to it
// to patch needs to be enabled on a route level, to do so add meta.patchCleanPath = true property to the route
// c.f. https://github.com/vuejs/router/issues/ 1638
Copy link
Member

Choose a reason for hiding this comment

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

remove blank in url please ;-)

@sonarcloud
Copy link

sonarcloud bot commented Jan 16, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

62.3% 62.3% Coverage
0.0% 0.0% Duplication

JammingBen
JammingBen approved these changes Jan 17, 2023
View reviewed changes
Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

LGTM in general, awesome work!

What's the reason we need to unref the current route now on many occurrences (e.g. this.$router.currentRoute)?

packages/web-app-files/tests/unit/components/AppBar/CreateAndUpload.spec.ts
@@ -12,6 +12,7 @@ import {
defaultStoreMockOptions,
defaultComponentMocks
} from 'web-test-helpers'
import { RouteLocation } from 'vue-router'
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather import this from web-test-helpers, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

I introduced RouteLocation in web-test-helpers only to avoid having to add vue-router as an explicit dependency in all packages only to make RouteLocation available in tests. So if there's already a dependency on vue-router it's fine to import it from there imho.

kulmann
kulmann approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Approving with the expectation, that we manage to fix the currently encoded slashes in the URL within the next couple of days. 😉

@kulmann kulmann merged commit 0edf878 into master Jan 17, 2023
@delete-merged-branch delete-merged-branch bot deleted the vue3-vue-router branch January 17, 2023 10:55
@dschmidt
Copy link
Member Author

Yes, I'll take care - thanks for merging, I'm really happy we avoid merge conflicts 😅

@dschmidt
Copy link
Member Author

What's the reason we need to unref the current route now on many occurrences (e.g. this.$router.currentRoute)?

Well, it's a Ref now 😅
In vue-router 3 it wasn't, that's why we had the 'useRoute()composable which usedrouter.afterEachto create aRefto thecurrentRoute. In vue-router 4 the library provides us with a Ref`

dschmidt added a commit that referenced this pull request Jan 18, 2023
@dschmidt
Vue 3: Fix encoded slashes in driveAliasAndItem
be2c6a5 
This is a follow up to #8215 where
we introduced a regression, so that slashes in file paths were encoded
in urls.
dschmidt added a commit that referenced this pull request Jan 18, 2023
@dschmidt
Vue 3: Fix encoded slashes in urls
faba482 
This is a follow up to #8215 where
we introduced a regression, so that slashes in file paths were encoded
in urls.
dschmidt added a commit that referenced this pull request Jan 18, 2023
@dschmidt
Vue 3: Fix encoded slashes in urls
f0d1d78 
This is a follow up to #8215 where
we introduced a regression, so that slashes in file paths were encoded
in urls.
@dschmidt dschmidt mentioned this pull request Jan 18, 2023
Merged
10 tasks
dschmidt added a commit that referenced this pull request Jan 19, 2023
@dschmidt
Vue 3: Fix encoded slashes in urls
bbc458a 
This is a follow up to #8215 where
we introduced a regression, so that slashes in file paths were encoded
in urls.
dschmidt added a commit that referenced this pull request Jan 19, 2023
@dschmidt
Vue 3: Fix encoded slashes in urls (#8269)
990e959 
This is a follow up to #8215 where
we introduced a regression, so that slashes in file paths were encoded
in urls.
ownclouders pushed a commit that referenced this pull request Jan 19, 2023
@dschmidt
commit 990e959
514ef08 
Author: Dominik Schmidt <[email protected]>
Date:   Thu Jan 19 09:23:26 2023 +0100

    Vue 3: Fix encoded slashes in urls (#8269)

    This is a follow up to #8215 where
    we introduced a regression, so that slashes in file paths were encoded
    in urls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JammingBen JammingBen JammingBen approved these changes

@kulmann kulmann kulmann approved these changes

@fschade fschade Awaiting requested review from fschade

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dschmidt @ownclouders @kulmann @fschade @JammingBen