bitwarden / clients

Bitwarden client apps (web, browser extension, desktop, and cli).
https://bitwarden.com
Other
8.65k stars 1.14k forks source link

[AC-2806] Add support for conditional routing based on feature flag value #9798

Closed eliykat closed 2 days ago

eliykat commented 5 days ago

🎟️ Tracking

https://bitwarden.atlassian.net/browse/AC-2806

📔 Objective

When refactoring a component, I want to make my changes to a copy of the component, and then route the user between the old and new components based on the value of a feature flag.

This PR facilitates this by creating a simple canMatchFn, which prevents a path from matching unless the feature flag matches a given state.

Thank you to @JaredSnider-Bitwarden for pointing out the canMatch functionality.

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 29.30%. Comparing base (76a3cb5) to head (709659b).

Files Patch % Lines
...ngular/src/platform/utils/feature-flagged-route.ts 0.00% 9 Missing :warning:
libs/angular/src/utils/component-route-swap.ts 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #9798 +/- ## ========================================== - Coverage 29.32% 29.30% -0.02% ========================================== Files 2527 2528 +1 Lines 73806 73813 +7 Branches 13781 13781 ========================================== - Hits 21641 21631 -10 - Misses 50540 50557 +17 Partials 1625 1625 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JaredSnider-Bitwarden commented 5 days ago

I'll pass the thanks along to @shane-melton for his excellent work in extension-refresh-route-utils.ts (#9004) which merely pointed out as an example!

github-actions[bot] commented 5 days ago

Logo Checkmarx One – Scan Summary & Detailsfbe2ee5e-a30d-49ef-9537-9082186c73e9

No New Or Fixed Issues Found

shane-melton commented 5 days ago

Just wanted to add that I added something similar in #9004 with the componentRouteSwap utility that's getting used as part of the browser refresh epic. I think it has the small benefit of allowing you to re-use the existing route configuration options for both routes so you don't need to define them twice.

I had left it generic originally it case we needed to evaluate other conditions that were not feature flag related, but I could see making a more specific utility around feature flags. For a more specific example, we have the extensionRefreshSwap helper for the browser refresh flag specifically.

eliykat commented 5 days ago

Neat! That's a nice extra helper, repurposing extensionRefreshSwap with an arbitrary feature flag would also work, and that might provide more structure for devs to use it properly. I could go either way though, part of me likes the extremely simple/transparent solution of providing the canMatchFn factory function with nothing else for general use. I'll wait for further feedback before making any changes; happy to do either.

eliykat commented 4 days ago

After sleeping on it, I like providing an out-of-the-box solution. I've adapted @shane-melton's existing work and made it work for any feature flag.

I wanted to write tests, however the usual way of testing routing is to use the RouterTestingModule, and that's effectively just a wrapper for mocks. We usually assert against the path, but that's identical for both components. I couldn't figure out any way to detect the actual component on the current route (again, because it's not performing any real navigation).

eliykat commented 2 days ago

Had to resolve conflicts with latest changes to componentRouteSwap, however those are otherwise non-breaking.