Stop clearing unrecognized query params #6784
Conversation
| @@ -148,6 +148,13 @@ export class DashboardDeepLinkProvider extends DeepLinkProvider { | |||
| return [{key: RUN_FILTER_KEY, value}]; | |||
| }) | |||
| ), | |||
| store | |||
There was a problem hiding this comment.
It would be nice (but not necessary) to also have this handled in the internal CorpExperimentListDeepLinkProvider (the CorpDashboardDeepLinkProvider, on the other hand, would get this for free already with the way you've coded this).
Not necessary to do but if you don't do it, I want to ensure it's a conscious choice rather than an oversight.
There was a problem hiding this comment.
I did remember CorpDashboardDeepLinkProvider and . I did not remember CorpExperimentListDeepLinkProvider but it should be simple enough to update once this makes it's way to google3
| @@ -358,10 +367,14 @@ describe('core deeplink provider', () => { | |||
| store.overrideSelector(selectors.getOverriddenFeatureFlags, { | |||
| enabledExperimentalPlugins: ['foo', 'bar', 'baz'], | |||
| }); | |||
| store.overrideSelector(selectors.getPluginQueryParams, { | |||
There was a problem hiding this comment.
Does it make sense for this to live in 'serializes feature flags'? Should it be its own test?
There was a problem hiding this comment.
I think it makes sense here because these are (were?) query parameters and they are being serialized.
If you feel strongly about it I'm happy to make another test.
There was a problem hiding this comment.
I don't understand what the additions to this test have to do with 'feature flags'. They don't seem necessary.
The comment for the test:
/**
* One test to verify that feature flags are correctly serialized using
* featureFlagsToSerializableQueryParams.
*/
The name for the test:
it('serializes feature flags', () => {
There was a problem hiding this comment.
I fully read "feature flags" as "query params". I guess working on TB has created an alias for me
## Motivation for features / changes The profiler plugin team wants to use query params for deeplinking within their application. Unfortunately we clear them when we re-serialize the application state. To support this I am adding a new area to the core state which will store unrecognized query parameters and re-serialize them. Googlers see cl/615196457 ## Screenshots of UI changes (or N/A) Time series still works!  The profiler plugin can read read these! 
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 as `localhost:6006/?showFlags=&showFlags=`. This change fixes this by deduplicating "unknown" flags with known feature flag query params.
Motivation for features / changes
The profiler plugin team wants to use query params for deeplinking within their application. Unfortunately we clear them when we re-serialize the application state. To support this I am adding a new area to the core state which will store unrecognized query parameters and re-serialize them.
Googlers see cl/615196457
Screenshots of UI changes (or N/A)
Time series still works!
The profiler plugin can read read these!
