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

fix: moved routing components to own high-level folder #997

Merged
merged 7 commits into from
Nov 28, 2024

Conversation

ckbedwell
Copy link
Contributor

Part 1 of addressing: #848

I plan on breaking the above ticket into separate PRs. Because of the nature of the work it is difficult to address up front how many there will be but because each PR will touch many files doing mundane things I intend to silo the work as much as possible to make review and discussion more relevant.

Problem

Addressing how the 'Routing' files are used and referenced as the current place they live is confusing, unintuitive and inconsistent.

Solution

  • Move 'Routing' files to their own high-level folder rather than being put in with 'components'
  • Add the 'routing' folder to the eslint auto-sorting folder structure
  • De-duplicate the utils files related to routing (we had routing/utils and routing/Routing.utils) - create a consistency of <FOLDER_AREA>/utils.ts
  • Move TestRouteInfo testing component to the tests folder (all test-related components should be easily discoverable in a single location rather than scattered around)

General

  • Change routing imports across files to absolute paths (at the end of the work I intend to add an eslint rule so everything has to be absolute paths)

@ckbedwell ckbedwell requested a review from w1kman November 25, 2024 16:46
@ckbedwell ckbedwell assigned ckbedwell and unassigned ckbedwell Nov 25, 2024
@ckbedwell ckbedwell requested a review from VikaCep November 25, 2024 16:46
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

IMO it's a bit confusing that SMDatasourceContext will return UninitialisedRouter.
Could we have one AppRouter that controls whether or not the uninitialized/initialized router should be used?

Thought
How about an index.ts file in src/routing to "package" and clean up imports.

src/routing/index.ts

export * from './InitialisedRouter';
export * from './UninitialisedRouter';
export * from './constants';
export * from './LegacyEditRedirect';
export * from './utils';

Example
After

import { InitialisedRouter, generateRoutePath, PLUGIN_URL_PATH } from 'routing`;

Before

import { InitialisedRouter } from 'routing/InitialisedRouter';
import { generateRoutePath } from 'routing/utils';
import { PLUGIN_URL_PATH } from 'routing/constants';

src/routing/InitialisedRouter.tsx Outdated Show resolved Hide resolved
src/test/render.tsx Outdated Show resolved Hide resolved
@ckbedwell
Copy link
Contributor Author

ckbedwell commented Nov 26, 2024

export * from './InitialisedRouter';
export * from './UninitialisedRouter';
export * from './constants';
export * from './LegacyEditRedirect';
export * from './utils';

The issue with this is jest and circular dependencies.

I was in the process of implementing this feedback and then I realised we have enum ROUTES in the global types file and I figured that was a good candidate for creating routing/types.ts file and placing there instead.

But if I export the types through the index too, Jest gets confused and errors out. If the files are imported individually this doesn't happen. We can revisit this in the future. I've had another issue with jest like this and had to make some awful 'temporary' shim.

IMO it's a bit confusing that SMDatasourceContext will return UninitialisedRouter.
Could we have one AppRouter that controls whether or not the uninitialized/initialized router should be used?

The problem with removing this responsibility from SMDatasourceProvider is you end up with the possibility that useSMDatasourceContext would return null or undefined which becomes really annoying. You might have a clearer picture in mind to handle this so we can explore it in another PR.

@ckbedwell ckbedwell requested a review from w1kman November 26, 2024 13:57
Copy link
Contributor

@VikaCep VikaCep left a comment

Choose a reason for hiding this comment

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

Nice! It makes much more sense to organize the routing files their own folder 👌

Comment on lines +1 to +16
export enum ROUTES {
Alerts = 'alerts',
CheckDashboard = 'checks/:id',
Checks = 'checks',
ChooseCheckGroup = 'checks/choose-type',
Config = 'config', // config (index)
EditCheck = 'checks/:id/edit',
ViewProbe = 'probes/:id',
EditProbe = 'probes/:id/edit',
Home = 'home',
NewCheck = 'checks/new',
NewProbe = 'probes/new',
Probes = 'probes',
Redirect = 'redirect',
Scene = 'scene',
}
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

@w1kman w1kman Nov 27, 2024

Choose a reason for hiding this comment

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

When are we going to address the naming of enums 😁
This enum at least uses PascalCase for keys.

// Bad 
enum AM_I_A_CONST_OR_A_ENUM {
  FOO,
  BAR,
}

// Bad 🤨
enum AM_I_A_CONST_OR_A_ENUM {
  Foo,
  Bar,
}

// Bad 🤨
enum AlmostAnEnum {
  FOO,
  BAR,
}

// Good 💖
enum ClearlyAnEnum {
  Foo,
  Bar,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1008 -- soon, made a ticket so we don't forget :)

@@ -3,7 +3,8 @@ import { GrafanaTheme2 } from '@grafana/data';
import { Icon, IconButton, LinkButton, Tooltip, useStyles2, useTheme2 } from '@grafana/ui';
import { css, cx } from '@emotion/css';

import { AlertSensitivity, Check, PrometheusAlertsGroup, ROUTES } from 'types';
import { AlertSensitivity, Check, PrometheusAlertsGroup } from 'types';
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that you moved the routing types away from the huge types.ts file. Is the idea to do this for most components in upcoming PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :D

Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

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

🥇 LGTM

@ckbedwell ckbedwell changed the title Refactor (1 of TBC): moved routing components to own high-level folder fix: moved routing components to own high-level folder Nov 28, 2024
@ckbedwell ckbedwell force-pushed the refactor/folder-structure/routing branch from b933e5c to 0ff6666 Compare November 28, 2024 09:18
@ckbedwell ckbedwell merged commit 02a0a95 into main Nov 28, 2024
5 checks passed
@ckbedwell ckbedwell deleted the refactor/folder-structure/routing branch November 28, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants