Fix Data Streams and Rollups Jobs deep-link bugs

[cjcenizal]

The original bug manifested itself when you'd click a data stream to view its details. Doing so would apply a filter to the table, which is a poor UX because the user would have to close the detail panel if they wanted to view the details of another data stream. This PR fixes this bug by requiring the ?isDeepLink=true query parameter to also be set in order to filter the table. This is set when you view a backing index's data stream and get deep-linked into the Data Streams tab, but not when you click a data stream to view its details while in that tab.

To enable this, I created the extractQueryParams service in es_ui_shared/public. When I did this, I discovered that we had duplicated this logic in CCR, Remote Clusters, and Rollup Jobs, so I also shaved the yak a bit and migrated these plugins to use this shared service.

Bonus: Rollup Jobs deep-link bugfix

As I did this I noticed a bug in Rollup Jobs (#70901). I fixed #70901 in a separate commit because it was a two-line change. These changes are unnecessarily coupled to one another so I can break them up if anyone thinks it's worth it. But I figured that if we needed to revert any of these changes it would be straightforward to do so manually.

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[jloleysens]

@elasticmachine merge upstream

[jloleysens]

Tested locally, and works as described. Great work @cjcenizal

Left a non-blocker comment.

[jloleysens]

Suggestion: I see the dataStreamName !== undefined logic existed before, but it might be worth changing here. This check is actually a lot more permissive than it needs to be (i.e., it tests true for all non-undefined values for dataStreamName). I would suggest dataStreamName != undefined.

No need to change if we have 100% confidence that dataStreamName will always be either a string or undefined.

[cjcenizal]

Good point, JL! Yes, I think we can be 100% confident it will always be either a string or undefined. It's provided by react router and is extracted from the path, so I'm not seeing any opportunities for it to be null, a number, or an object. I think we can leave as-is.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 74bd3aa

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
esUiShared	104	+4	100
remoteClusters	66	-4	70
total	-	-0	-

History

• 💔 Build #59221 failed cfe0b7a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[sebelga]

@cjcenizal @jloleysens I think we need to write some guidelines for our team about how to export from "es_ui_shared". I thought that we were aligned but I see we are not, and that's probably my mistake of not spreading the word.

The guidelines to follow, like we can see on L71, is to export namespace packages from the es_ui_shared folder. This implies to group items by "domain". Here I would have suggested having a "Routing" domain.

Each domain represents a top-level folder that lives inside the "packages_do_not_import" folder.

My main concerns are:

• name collision. I don't want to have to tweak my interface names to not collide with other interfaces from other existing domains. If UpdateHandler makes sense in my context, I want to be able to use it, even if another domain is using it in a different context.

• Pollute the public index.ts with all the interfaces, constants, functions from all the packages. This file will keep growing with time and it's much better to export "packages" like we do with

export { Monaco, Forms, ace };

If we need to know what a package contains, we then go and look at its barrel file inside its folder.

Until the guidelines are written I think it is important to keep an eye on that during code reviews. WDYT?

[jloleysens]

@sebelga , I do agree with this overall. I think it is best in as far as possible to follow the convention you've outlined (which is why I created #72034 in response to your comment :) ).

In this case naming of the function seemed so specific that I was not worried collision protection. That being said it would probably have been best if I'd also called it out here.

The other aspect of putting something that is fairly uniquely named inside of a module like Routing.extractQueryParams is the added step of unwrapping it later or updating all uses to have the module name prefix. On my initial review I just thought it might be overkill.

But again, in retrospect, I think it should have been called it attention here.

[sebelga]

I was not pointing you directly about the review 😊 Just sharing thoughts/concerns. Thanks for creating #72034 !

The thing about thinking "naming of the function seemed so specific that I was not worried collision protection" is that it is subject to interpretation so I think it is better to follow a pattern and stick to it.

For the extra unwrapping, yes there is an extra step but I think it is worth in the long run. True that we might need to update the code in a few places but I didn't find it too complex to do when I worked with Forms

// instead of this
import { FormWizard } from '../../shared_imports';

// we have this
import { Forms } from '../../shared_imports';
const { FormWizard } = Forms;

IMO it makes it very clear what reusable package(s) a file uses and we can understand the context of a specific handler/interface.