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

feat: API for reloading current route target #19331

Merged
merged 7 commits into from May 15, 2024

Conversation

tepi
Copy link
Contributor

@tepi tepi commented May 8, 2024

Fixes #19244

Prepares implementation of #19262

Copy link

github-actions bot commented May 8, 2024

Test Results

1 105 files  +2  1 105 suites  +2   1h 20m 43s ⏱️ - 1m 41s
7 020 tests +8  6 971 ✅ +8  49 💤 ±0  0 ❌ ±0 
7 390 runs  +1  7 331 ✅ +1  59 💤 ±0  0 ❌ ±0 

Results for commit 38af05a. ± Comparison against base commit 8b803ad.

♻️ This comment has been updated with latest results.

@tepi tepi marked this pull request as draft May 8, 2024 11:05
@tepi
Copy link
Contributor Author

tepi commented May 8, 2024

Draft for now - just wanted to see existing tests pass fine with the change. Still needs tests to be added for the refresh functionality.

@tepi tepi marked this pull request as ready for review May 8, 2024 13:33
@tepi
Copy link
Contributor Author

tepi commented May 13, 2024

Something to consider is @PreserveOnRefresh routes - what should we do with them? This PR basically bypasses them, so even if explicit reload API is called, the cached instance will still be used. We could recreate those as well if it seems appropriate.

@mshabarov mshabarov requested a review from mcollovati May 13, 2024 11:41
@mcollovati
Copy link
Collaborator

Something to consider is @PreserveOnRefresh routes - what should we do with them? This PR basically bypasses them, so even if explicit reload API is called, the cached instance will still be used. We could recreate those as well if it seems appropriate.

My opinion is that the new API should recreate components even if the route is marked with @PreserveOnRefresh. The new method performs an action that is different from a normal page reload or navigation, so if a developer is explicitly calling it, I think it would expect route component(s) to be recreated, even if preserve on refresh is in use.

@tepi
Copy link
Contributor Author

tepi commented May 14, 2024

Logic in this PR recreates the whole layout chain, which might not be wanted in all cases. Working on a change to provide means to control this in the API.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels May 14, 2024
@vaadin-bot vaadin-bot added +0.0.1 and removed +1.0.0 labels May 14, 2024
@tepi tepi requested a review from mcollovati May 15, 2024 08:35
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels May 15, 2024
Copy link

sonarcloud bot commented May 15, 2024

Quality Gate Passed Quality Gate passed

Issues
58 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tepi tepi merged commit 0c4168e into main May 15, 2024
26 checks passed
@tepi tepi deleted the feat/api-to-reload-current-route branch May 15, 2024 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reload/refresh router content without reloading the full page
3 participants