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

Suggestions for some method signature changes #6738

Closed
ByungjunYou opened this issue Mar 29, 2024 · 2 comments
Closed

Suggestions for some method signature changes #6738

ByungjunYou opened this issue Mar 29, 2024 · 2 comments

Comments

@ByungjunYou
Copy link

ByungjunYou commented Mar 29, 2024

After reading the below code in feature-toggle-strategies-store.ts, I was confused with some method signatures.

I think the return type of getFeatureOverviewData should be Record<string, IFeatureOverview> instead of IFeatureOverview. Because acc[row.feature_name] = { type: ..., description: ... } suggests that acc is a record whose key is string type and value is IFeatureOverview type.

getFeatureOverviewData(rows): IFeatureOverview {
return rows.reduce((acc, row) => {
if (acc[row.feature_name]) {
const environmentExists = acc[
row.feature_name
].environments.some(
(existingEnvironment) =>
existingEnvironment.name === row.environment,
);
if (!environmentExists) {
acc[row.feature_name].environments.push(
FeatureStrategiesStore.getEnvironment(row),
);
}
if (this.isNewTag(acc[row.feature_name], row)) {
this.addTag(acc[row.feature_name], row);
}
} else {
acc[row.feature_name] = {
type: row.type,
description: row.description,
favorite: row.favorite,
name: row.feature_name,
createdAt: row.created_at,
stale: row.stale,
impressionData: row.impression_data,
lastSeenAt: row.last_seen_at,
environments: [FeatureStrategiesStore.getEnvironment(row)],
};
if (this.isNewTag(acc[row.feature_name], row)) {
this.addTag(acc[row.feature_name], row);
}
}
const featureRow = acc[row.feature_name];
if (
isAfter(
new Date(row.env_last_seen_at),
new Date(featureRow.lastSeenAt),
)
) {
featureRow.lastSeenAt = row.env_last_seen_at;
}
return acc;
}, {});
}

export interface IFeatureOverview {
name: string;
description: string;
project: string;
favorite: boolean;
impressionData: boolean;
segments: string[];
type: string;
stale: boolean;
createdAt: Date;
lastSeenAt: Date;
environments: IEnvironmentOverview[];
}

In case of the parameter type of sortEnvironments, it also might be correct to use Record<string, IFeatureOverview>. Since Object.values(overview).map((data: IFeatureOverview) strongly suggest that overview is a record whose value is IFeatureOverview type.

const sortEnvironments = (overview: IFeatureOverview) => {
return Object.values(overview).map((data: IFeatureOverview) => ({
...data,
environments: data.environments
.filter((f) => f.name)
.sort((a, b) => {
if (a.sortOrder === b.sortOrder) {
return a.name.localeCompare(b.name);
}
return a.sortOrder - b.sortOrder;
}),
}));
};

If the suggestions make sense to you, I'm also open to make a PR for the fix. :)

@sjaanus
Copy link
Contributor

sjaanus commented Apr 2, 2024

Good catch, addressed it here #6750

@ByungjunYou
Copy link
Author

@sjaanus
Thank you for the fix :)

sjaanus added a commit that referenced this issue Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

2 participants