-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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.
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
ref(js): Small code cleanup to ProjectsStore #29595
Conversation
dc326fc to
84b19f0
Compare
| map[project.id] = project; | ||
| return map; | ||
| }, {}); | ||
| const mapping = items.map(project => [project.id, project] as const); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll update 👍
| // Assign stats into projects | ||
| entries.forEach(([projectId, stats]) => { | ||
| this.itemsById[projectId].stats = stats; | ||
| }); | ||
|
|
||
| const touchedIds = entries.map(([projectId]) => projectId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 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; | |
| }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| projects.forEach(project => this.removeTeamFromProject(teamSlug, project)); | ||
|
|
||
| const affectedProjectIds = projects.map(project => project.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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; | |
| }); |
No description provided.