-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Deduplication of entries and items before sending to endpoint #71297
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,10 +97,18 @@ export function translateToEndpointExceptions( | |
exc: FoundExceptionListItemSchema, | ||
schemaVersion: string | ||
): TranslatedExceptionListItem[] { | ||
const entrySet = new Set(); | ||
const entriesFiltered: TranslatedExceptionListItem[] = []; | ||
if (schemaVersion === 'v1') { | ||
return exc.data.map((item) => { | ||
return translateItem(schemaVersion, item); | ||
exc.data.forEach((entry) => { | ||
const translatedItem = translateItem(schemaVersion, entry); | ||
const entryHash = createHash('sha256').update(JSON.stringify(translatedItem)).digest('hex'); | ||
if (!entrySet.has(entryHash)) { | ||
entriesFiltered.push(translatedItem); | ||
entrySet.add(entryHash); | ||
} | ||
}); | ||
return entriesFiltered; | ||
} else { | ||
throw new Error('unsupported schemaVersion'); | ||
} | ||
|
@@ -124,12 +132,17 @@ function translateItem( | |
schemaVersion: string, | ||
item: ExceptionListItemSchema | ||
): TranslatedExceptionListItem { | ||
const itemSet = new Set(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again you could use a custom accumulator here to avoid the global state... not sure how much of a problem it is, maybe get another opinion on it. Looks like it will work though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not too well versed on this. Does this provide us with benefits other than syntactic sugar/style? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexk307 I'm not the best authority on javascript, but my understanding is that it's best practice to avoid mutable state and mixed scope. This might not be so bad because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this too. |
||
return { | ||
type: item.type, | ||
entries: item.entries.reduce((translatedEntries: TranslatedEntry[], entry) => { | ||
const translatedEntry = translateEntry(schemaVersion, entry); | ||
if (translatedEntry !== undefined && translatedEntryType.is(translatedEntry)) { | ||
translatedEntries.push(translatedEntry); | ||
const itemHash = createHash('sha256').update(JSON.stringify(translatedEntry)).digest('hex'); | ||
if (!itemSet.has(itemHash)) { | ||
translatedEntries.push(translatedEntry); | ||
itemSet.add(itemHash); | ||
} | ||
} | ||
return translatedEntries; | ||
}, []), | ||
|
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 think this would be cleaner using
reduce
with an accumulator that contains both the set and the filtered list... and then just return the list at the end. Or you could usefilter
, but usingreduce
could avoid the external state.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 was going to use filter but avoided it because I thought it would just make things worse since I have to add to the set as well after checking if it exists.
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 think filter or reduce would be preferred over forEach here. My guess is that reduce will be the best fit, but again, I'm still learning.
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 agree with @madirey here. Using reduce as she describes would be cleaner. However, I think it would be better if we pulled out the super duper deduping logic out of this function and into a separate function. That way we could test that logic independently.
translateToEndpointExceptions
andtranslateItem
should do only that, translation. Deduping might be better handled elsewhere.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.
Not sure if I agree, pulling this out would still force us to keep some sort of state within the translation functions, and just move the
Set
methods elsewhere. Not much use in testing that separately I think.I'll look into filter or reduce
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.
We talked and now we like this. 💯