Skip to content

fix(manager-pci-common): add undefined type to the project on isDisco… #17728

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

seven-amid
Copy link
Contributor

…veryProject

ref: #17727

…veryProject

ref: #17727

Signed-off-by: LIDRISSI Hamid <lidrissi.hamid@gmail.com>
@seven-amid seven-amid requested a review from a team as a code owner June 19, 2025 13:21
@github-actions github-actions bot added the bug Something isn't working label Jun 19, 2025
@seven-amid seven-amid linked an issue Jun 19, 2025 that may be closed by this pull request
1 task
@@ -11,7 +11,7 @@ import '../../../translations/discovery-banner';

const DISCOVERY_PROJECT_PLAN_CODE = 'project.discovery';

export const isDiscoveryProject = (project: TProject) =>
export const isDiscoveryProject = (project: TProject | undefined) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const isDiscoveryProject = (project: TProject | undefined) =>
export const isDiscoveryProject = (project?: TProject) =>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the same idea no ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the first one is verbose regarding how TS defines optional parameters (https://www.typescriptlang.org/docs/handbook/2/functions.html#optional-parameters)
it's up to you as of course it's non blocking ;)

@@ -11,7 +11,7 @@ import '../../../translations/discovery-banner';

const DISCOVERY_PROJECT_PLAN_CODE = 'project.discovery';

export const isDiscoveryProject = (project: TProject) =>
export const isDiscoveryProject = (project: TProject | undefined) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

For me we should not have an undefined project at this step. You should catch errors at the top app level, and throughout your entire app consider that the project exists.

Because with this implementation we check multiple times in each banner and screen if a project exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[manager-pci-common]: <PciDiscoveryBanner component>
3 participants