ref(js): spring cleaning - #48116
Conversation
github-actions
Bot
added
the
Scope: Frontend
| const selectedIssues: BaseGroup[] = []; | ||
| for (const issue of issues) { | ||
| const resolvedIssue = GroupStore.get(issue); | ||
|
|
||
| if (resolvedIssue) { | ||
| // @TODO use a type guard here | ||
| selectedIssues.push(resolvedIssue as BaseGroup); | ||
| } | ||
| } |
There was a problem hiding this comment.
could just switch it to issues.map so there isn't an extra array being made
There was a problem hiding this comment.
Actually, set.forEach is better here, same with below comment I think
| const projects: string[] = []; | ||
|
|
||
| for (const selectedGroup of selected) { | ||
| const group = GroupStore.get(selectedGroup); | ||
|
|
||
| if (group && group.project) { | ||
| projects.push(group.project.slug); | ||
| } | ||
| } |
There was a problem hiding this comment.
i feel like the map and filter should just be written better
maybe
const projects = selected
.map(id => GroupStore.get(id)?.project?.slug)
.filter(defined)There was a problem hiding this comment.
It does not work because selected is a Set :/
| newProject = memberProjects?.[0] | ||
| ? [getProjectIdFromProject(memberProjects[0])] | ||
| : []; |
There was a problem hiding this comment.
undefined is not iterable, thus if memberProjects is undefined,[...memberProjects] would already fail earlier anyway, which mean we can write :P
| newProject = memberProjects?.[0] | |
| ? [getProjectIdFromProject(memberProjects[0])] | |
| : []; | |
| newProject = [getProjectIdFromProject(memberProjects[0])] |
There was a problem hiding this comment.
I think what happens is that calling memberProjects with undefined throws (least from looking at the test failures), so I'd need to protect from that. I wrote that exact line at first hehe
| const emptyValue = mappedKeys.reduce((a, v) => { | ||
| a[v] = null; | ||
| return a; | ||
| }, {}); |
There was a problem hiding this comment.
Alternatively:
| const emptyValue = mappedKeys.reduce((a, v) => { | |
| a[v] = null; | |
| return a; | |
| }, {}); | |
| const emptyValue = Object.fromEntries(mappedKeys.map(key => ([key, null]))); |
There was a problem hiding this comment.
Oh nice, I like that. I think we had some errors around Object.fromEntries in unsupported browsers though, I'd need to check
|
|
||
| const mappedKeys = columnKeys || []; | ||
| const emptyValue = mappedKeys.reduce((a, v) => ({...a, [v]: null}), {id: ''}); | ||
| const emptyValue = mappedKeys.reduce( |
There was a problem hiding this comment.
Same as above, but with .concat(['id', '']) at the end of map.
| list.reduce<SDKUpdatesSuggestion[]>((suggestions, sdk) => { | ||
| return suggestions.concat(sdk.suggestions); | ||
| }, []); |
There was a problem hiding this comment.
| list.reduce<SDKUpdatesSuggestion[]>((suggestions, sdk) => { | |
| return suggestions.concat(sdk.suggestions); | |
| }, []); | |
| list.flatMap(l => l.suggestions) as SDKUpdatesSuggestion[]; |
(not sure if type assertion is required)
There was a problem hiding this comment.
Ah yes, much nicer. I keep forgetting about flatMap
0ab4326 to
a296008
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
3 participants
I have been experimenting with some eslint rules to detect performance issues as per getsentry/sentry-javascript#7910 and #47629. All of this is pretty much in the same spirit where we try to find On^2 code + unnecessary array rest/spread.
Figured it wouldn't hurt to clean some of the code in Sentry anyways so I'm opening this as draft and awareness. I would not expect any of these to be actual performance issues, but the code is still wasteful and in some cases just less readable (like the ...set.length vs just checking set.size)
Lmk what you think, I'd be curious to hear some thoughts and opinions, the only thing to cleanup is just the chang to eslintrc which was linked for testing purposes