Dedupe unknown query params from feature flag query params. #6824
Conversation
| ); | ||
| return ( | ||
| Object.entries(queryParams) | ||
| // deserializeQueryParams doesn't check feature flag metadata before |
There was a problem hiding this comment.
I don't have the context here, but isn't this the underlying issue instead? Can we make it so "non-unknown" parameters are not recognized as "unknown"? Or perhaps, does this come from the internal/external code separation?
There was a problem hiding this comment.
To Adrian's point, can we read the feature flag keys from FeatureFlagMetadataMap and ignore them in deserializeQueryParams?
Another idea: WDYT of filtering out all known keys (not just feature flags) from the unknown flag list (something like this) as a guard against similar bugs in the future? (And maybe log a warning that this is happening because it's probably an indication that deserializeQueryParams needs to be updated)
There was a problem hiding this comment.
Not going to lie: I considered fixing this in deserializeQueryParams but decided not to because it would require a lot more effort. I'm trying to fix this with the limited amount of time I have.
The reason: It requires an API change to DeepLinkProvider, which has implementations in the internal code base. The ideal API change, to maintain symmetry with serializeStateToQueryParams, would be to:
deserializeQueryParams(
store: Store<State>
queryParams: SerializableQueryParams
): Observable<DeserializedState>
This requires changing how it is called in app_routing_effects.ts. This requires a dance over several PRs in order to get it to import cleanly. It requires I update all related tests (app_routing_effects and all the implementations of DeepLinkProvider) to work with an async Observable interface.
I think it is slightly easier if I make the compromise of changing the API to:
deserializeQueryParams(
featureFlags: FeatureFlagMetadata
queryParams: SerializableQueryParams
): DeserializedState
And instead get app_routing_effects to retrieve the FeatureFlagMetadata. But only slightly easier.
Another idea: WDYT of filtering out all known keys (not just feature flags) from the unknown flag list
Done. Thanks for the code.
There was a problem hiding this comment.
Right, I hadn't anticipated these refactoring hurdles. I agree should focus on fixing the bug first, which seems to be making showFlags= unusable in local dev.
Thanks for creating the fix!
| showFlags: '', | ||
| }); | ||
| store.overrideSelector(selectors.getUnknownQueryParams, { | ||
| foo: 'bar', |
There was a problem hiding this comment.
Are these foo and bar query params related to the value of the enabledExperimentalPlugins flag?
I'm not sure I follow what is being tested here. What is being deduped? The showFlags one? Are the other flags relevant for the test, then?
There was a problem hiding this comment.
I tried to make the test clearer and not reuse 'foo' and 'bar'. Let me know if it makes more sense now.
There was a problem hiding this comment.
The new tests are clearer in that regard, thanks!
| return queryParamList.flat(); | ||
| combineLatestWith(store.select(selectors.getUnknownQueryParams)), | ||
| map(([queryParamList, unknownQueryParams]) => { | ||
| // Filter out any known params from unknownQueryParams. |
There was a problem hiding this comment.
Would it be appropriate to add details of why there might be "known" parameters in the "unknown" parameters?
I agree that we don't need to invest too much time in a "proper" solution that takes much more effort, but I'd like for future readers of this code (including us) to be aware of what the underlying issue is.
Admittedly, I'm not familiar enough with the code to understand how this would be solved better, or what precisely causes this, so I'll leave it to your judgement to decide if it would be useful, but a short comment can help a lot sometimes.
| unknown2: 'unknown2_value', | ||
| // Should be ignored since it is also a feature flag query param. | ||
| experimentalPlugin: | ||
| 'this_is_known_but_has_different_value_for_some_reason', |
There was a problem hiding this comment.
Curious: do we know that we would want to use the value set in the "known" parameters and ignore the other one? Or is it the case that we can't know, but this is just a simple way of deduping? Or perhaps, we know in some scenarios, but not all of them?
Since #6784 we have seen the unusual behavior where query params for feature flags will be duplicated on the command line.
For example, if we load
localhost:6006/?showFlags=it will be rewritten aslocalhost:6006/?showFlags=&showFlags=.This change fixes this by deduplicating "unknown" flags with known feature flag query params.