Skip to content

ref(js): Small code cleanup to ProjectsStore #29595

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.

Sign up for GitHub

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

Merged
evanpurkhiser merged 1 commit into from
Nov 2, 2021
Merged

ref(js): Small code cleanup to ProjectsStore #29595

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 34 additions & 61 deletions static/app/stores/projectsStore.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type StatsData = Record<string, Project['stats']>;
* Attributes that need typing but aren't part of the external interface,
*/
type Internals = {
removeTeamFromProject(teamSlug: string, project: Project): void;
itemsById: Record<string, Project>;
loading: boolean;
};
Expand All @@ -32,9 +33,7 @@ type ProjectsStoreInterface = CommonStoreInterface<State> & {
onDeleteTeam(slug: string): void;
onRemoveTeam(teamSlug: string, projectSlug: string): void;
onAddTeam(team: Team, projectSlug: string): void;
removeTeamFromProject(teamSlug: string, project: Project): void;
isLoading(): boolean;
getWithTeam(teamSlug: string): Project[];
getAll(): Project[];
getById(id?: string): Project | undefined;
getBySlug(slug?: string): Project | undefined;
Expand Down Expand Up @@ -65,66 +64,56 @@ const storeConfig: Reflux.StoreDefinition & Internals & ProjectsStoreInterface =
},

loadInitialData(items: Project[]) {
this.itemsById = items.reduce((map, project) => {
map[project.id] = project;
return map;
}, {});
const mapping = items.map(project => [project.id, project] as const);
Copy link
Member

@priscilawebdev priscilawebdev Oct 27, 2021

Choose a reason for hiding this comment

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

nit: shouldn't we call items projects?

Copy link
Member Author

@evanpurkhiser evanpurkhiser Oct 28, 2021

Choose a reason for hiding this comment

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

Yeah I'll update 👍


this.itemsById = Object.fromEntries(mapping);
this.loading = false;

this.trigger(new Set(Object.keys(this.itemsById)));
},

onChangeSlug(prevSlug: string, newSlug: string) {
const prevProject = this.getBySlug(prevSlug);

// This shouldn't happen
if (!prevProject) {
return;
}

const newProject = {
...prevProject,
slug: newSlug,
};

this.itemsById = {
...this.itemsById,
[newProject.id]: newProject,
};
const newProject = {...prevProject, slug: newSlug};

// Ideally we'd always trigger this.itemsById, but following existing patterns
// so we don't break things
this.itemsById = {...this.itemsById, [newProject.id]: newProject};
this.trigger(new Set([prevProject.id]));
},

onCreateSuccess(project: Project) {
this.itemsById = {
...this.itemsById,
[project.id]: project,
};
this.itemsById = {...this.itemsById, [project.id]: project};
this.trigger(new Set([project.id]));
},

onUpdateSuccess(data: Partial<Project>) {
const project = this.getById(data.id);

if (!project) {
return;
}
const newProject = Object.assign({}, project, data);
this.itemsById = {
...this.itemsById,
[project.id]: newProject,
};

const newProject = {...project, ...data};

this.itemsById = {...this.itemsById, [project.id]: newProject};
this.trigger(new Set([data.id]));
},

onStatsLoadSuccess(data) {
const touchedIds: string[] = [];
Object.entries(data || {}).forEach(([projectId, stats]) => {
if (projectId in this.itemsById) {
this.itemsById[projectId].stats = stats;
touchedIds.push(projectId);
}
const entries = Object.entries(data || {}).filter(
([projectId]) => projectId in this.itemsById
);

// Assign stats into projects
entries.forEach(([projectId, stats]) => {
this.itemsById[projectId].stats = stats;
});

const touchedIds = entries.map(([projectId]) => projectId);
Comment on lines +111 to +116
Copy link
Member

@priscilawebdev priscilawebdev Oct 27, 2021

Choose a reason for hiding this comment

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

Suggested change
// Assign stats into projects
entries.forEach(([projectId, stats]) => {
this.itemsById[projectId].stats = stats;
});
const touchedIds = entries.map(([projectId]) => projectId);
const touchedIds = entries.map(([projectId, stats]) => {
// Assign stats into projects
this.itemsById[projectId].stats = stats;
return projectId;
});

Copy link
Member Author

@evanpurkhiser evanpurkhiser Oct 27, 2021

Choose a reason for hiding this comment

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

lol this is what it did before, but I hated that a map had side effects

Copy link
Member

@priscilawebdev priscilawebdev Oct 28, 2021

Choose a reason for hiding this comment

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

I understand, but this way, we don't need to do a 2x loop. Was the side effect negatively affecting anything?

Copy link
Member Author

@evanpurkhiser evanpurkhiser Oct 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The performance difference is completely negligible. (O(2n) is the same as O(n))

The side effect was not affecting anything, but IMO it is harder to read.

From this article: https://glebbahmutov.com/blog/avoid-side-effects-with-immutable-data-structures/

When using immutable data structures instead of JavaScript native objects, our code and data flow becomes easier to understand and reason

this.trigger(new Set(touchedIds));
},

Expand All @@ -135,16 +124,19 @@ const storeConfig: Reflux.StoreDefinition & Internals & ProjectsStoreInterface =
*/
onDeleteTeam(teamSlug: string) {
// Look for team in all projects
const projectIds = this.getWithTeam(teamSlug).map(projectWithTeam => {
this.removeTeamFromProject(teamSlug, projectWithTeam);
return projectWithTeam.id;
});
const projects = this.getAll().filter(({teams}) =>
teams.find(({slug}) => slug === teamSlug)
);

this.trigger(new Set([projectIds]));
projects.forEach(project => this.removeTeamFromProject(teamSlug, project));

const affectedProjectIds = projects.map(project => project.id);
Comment on lines +131 to +133
Copy link
Member

@priscilawebdev priscilawebdev Oct 27, 2021

Choose a reason for hiding this comment

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

Suggested change
projects.forEach(project => this.removeTeamFromProject(teamSlug, project));
const affectedProjectIds = projects.map(project => project.id);
const affectedProjectIds = projects.map(project => {
this.removeTeamFromProject(teamSlug, project);
return project.id;
});

this.trigger(new Set(affectedProjectIds));
},

onRemoveTeam(teamSlug: string, projectSlug: string) {
const project = this.getBySlug(projectSlug);

if (!project) {
return;
}
Expand All @@ -161,37 +153,18 @@ const storeConfig: Reflux.StoreDefinition & Internals & ProjectsStoreInterface =
return;
}

this.itemsById = {
...this.itemsById,
[project.id]: {
...project,
teams: [...project.teams, team],
},
};
const newProject = {...project, teams: [...project.teams, team]};

this.itemsById = {...this.itemsById, [project.id]: newProject};
this.trigger(new Set([project.id]));
},

// Internal method, does not trigger
removeTeamFromProject(teamSlug: string, project: Project) {
const newTeams = project.teams.filter(({slug}) => slug !== teamSlug);
const newProject = {...project, teams: newTeams};

this.itemsById = {
...this.itemsById,
[project.id]: {
...project,
teams: newTeams,
},
};
},

/**
* Returns a list of projects that has the specified team
*
* @param {String} teamSlug Slug of team to find in projects
*/
getWithTeam(teamSlug: string) {
return this.getAll().filter(({teams}) => teams.find(({slug}) => slug === teamSlug));
this.itemsById = {...this.itemsById, [project.id]: newProject};
},

isLoading() {
Expand Down