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

[ML] WIP - Extend MlUrlGenerator & convert ML urls to non-hash #74985

Closed
wants to merge 6 commits into from

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Aug 13, 2020

Summary

Part of #69265

This DRAFT PR is a WIP attempt to consolidate the url generating service within the ML plugin.

Current it's contingent on this PR #74955 being merged in, but the original commit from that PR is being included here to prevent typescript from breaking.

It's not currently replacing everything yet, just snippets to see how it would work if replaced. For example, it can be used by other team's with Kibana's navigateToUrl

  const urlGenerator = useMlUrlGenerator();
  const {
    services: {
      application: { navigateToUrl },
    },
  } = useMlKibana();

  const redirectToAnalyticsManagementPage = async () => {
    const url = await urlGenerator.createUrl({ page: ML_TABS.DATA_FRAME_ANALYTICS });
    await navigateToUrl(url);
  };

or our own Ml's navigateToPath which can optionally preserve the search queries

  const urlGenerator = useMlUrlGenerator();
  const navigateToPath = useNavigateToPath();

  const redirectToAnalyticsManagementPage = async () => {
    const path = await urlGenerator.createUrl({
      page: 'data_frame_analytics/exploration',
      jobId,
      analysisType,
    });
    await navigateToPath(path, preserveSearch);
  };

@qn895 qn895 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 13, 2020
@qn895 qn895 self-assigned this Aug 13, 2020
@@ -22,6 +22,23 @@ import { stringify, ParsedQuery } from 'query-string';
import { parseUrl, parseUrlHash } from './parse';
import { url as urlUtils } from '../../../common';

export function replaceUrlQuery(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should change the content of kibana_utils

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That change is now merged in here #74955.

@@ -65,40 +163,214 @@ export class MlUrlGenerator implements UrlGeneratorsDefinition<typeof ML_APP_URL
public readonly id = ML_APP_URL_GENERATOR;

public readonly createUrl = async ({ page, ...params }: MlUrlGeneratorState): Promise<string> => {
if (page === 'explorer') {
return this.createExplorerUrl(params);
if (page === ML_TABS.ANOMALY_DETECTION) {
Copy link
Contributor

@darnautov darnautov Aug 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove destructuring from the function arguments, and change to something like

Suggested change
if (page === ML_TABS.ANOMALY_DETECTION) {
public readonly createUrl = async (mlUrlGeneratorState: MlUrlGeneratorState): Promise<string> => {
switch(mlUrlGeneratorState) {
case mlUrlGeneratorState.page === ML_TABS.ANOMALY_DETECTION:
return this.createAnomalyDetectionJobManagementUrl(mlUrlGeneratorState)
}

in that case, TS will take care of typecasting and there is no need to do it manually.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good spot. And with this way it's much cleaner too! Thanks 🙏

@qn895 qn895 marked this pull request as ready for review August 21, 2020 15:58
@qn895 qn895 requested review from a team as code owners August 21, 2020 15:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 21, 2020

💔 Build Failed

Failed CI Steps


Test Failures

X-Pack Jest Tests.x-pack/plugins/lens/public/app_plugin.Lens App sets originatingApp breadcrumb when the document title changes

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 1 times on tracked branches: https://github.com/elastic/kibana/issues/75694


Stack Trace

Error: expect(jest.fn()).toHaveBeenCalledWith(...expected)

- Expected
+ Received

  Array [
    Object {
-     "onClick": Anything,
+     "onClick": [Function onClick],
      "text": "The Coolest Container Ever Made",
    },
    Object {
      "href": "/testbasepath/app/visualize#/",
-     "onClick": Anything,
+     "onClick": [Function onClick],
      "text": "Visualize",
    },
    Object {
-     "text": "Daaaaaaadaumching!",
+     "text": "Create",
    },
  ],

Number of calls: 1
    at Object.it (/dev/shm/workspace/parallel/10/kibana/x-pack/plugins/lens/public/app_plugin/app.test.tsx:328:52)

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
ml 1374 +1 1373

async chunks size

id value diff baseline
ml 8.2MB +816.0B 8.2MB

page load bundle size

id value diff baseline
ml 583.7KB +7.4KB 576.4KB

History

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

@qn895 qn895 closed this Aug 21, 2020
@qn895 qn895 deleted the ml-url-generator-refactor branch August 21, 2020 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants