-
Notifications
You must be signed in to change notification settings - Fork 144
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
✨ Allow to modify source_url property in Loaf script attributions #3325
✨ Allow to modify source_url property in Loaf script attributions #3325
Conversation
075b77b
to
b386e8b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3325 +/- ##
==========================================
+ Coverage 92.94% 92.96% +0.01%
==========================================
Files 297 297
Lines 7840 7833 -7
Branches 1785 1785
==========================================
- Hits 7287 7282 -5
+ Misses 553 551 -2 ☔ View full report in Codecov by Sentry. |
b386e8b
to
db7d41f
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
const newType = getType(newValue) | ||
if (newType === fieldType) { | ||
set(object, fieldPath, sanitize(newValue)) | ||
const pathSegments = fieldPath.split('.') |
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 love the approach! The one suggestion I have is to tweak the implementation strategy slightly. If you change this line to:
const pathSegments = fieldPath.split('.') | |
const pathSegments = fieldPath.split(/\.|(?=\[\])/) |
Then instead of foo.bar[].baz
being split into ['foo', 'bar[]', 'baz']
, it will be split into ['foo', 'bar', '[]', 'baz']
. This should lead to a cleaner implementation of getValueFromPath()
, where you can treat []
as a special field name that maps over all elements of an array. With the appropriate changes to getValueFromPath()
, it will also mean that foo.bar[][].baz
will work as expected with no special effort, so that we can support modifications of nested arrays.
|
||
function isValidType(actualType: string | string[], expectedType: 'string' | 'object'): boolean { | ||
if (Array.isArray(actualType)) { | ||
return actualType.length > 0 && actualType.every((type) => type === expectedType) |
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 don't validate here that the length matches; is that expected? I may be misreading the code, but it seems like this will allow people to remove items from the array while still passing the isValidType()
check; the result looks like it would be that you'd get undefined
for some elements in the output array. That may be OK, but I just wanted to make sure it was intentional. (Or that it's handled somehow and I'm just missing it.)
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.
You're right, I haven't consider this. Actually, it also means that every property of the clone is a valid type. If only some are, none will be copied to the original object.
I've come up with a better algorithm, that should be resilient to such inconsistencies in the clone: traverse both, the object and the clone recursively at the same time, up to the end of the path where I can check the path individually.
At the end, I think the code is simpler and even more efficient as we now do the get and set traversal at once
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 really like the final result! Nicely done.
if (field === '[]') { | ||
if (!Array.isArray(object) || !Array.isArray(clone)) { | ||
return | ||
} | ||
current = current[field] | ||
|
||
object.forEach((item, i) => setValueAtPath(item, clone[i], restPathSegments, fieldType)) | ||
return | ||
} |
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.
🥜 nitpick:
if (field === '[]') {
if (Array.isArray(object) && Array.isArray(clone)) {
object.forEach((item, i) => setValueAtPath(item, clone[i], restPathSegments, fieldType));
}
return;
}
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.
Looks good! I really like the final solution you came up with; very clean!
Motivation
We want to be able to modify
source_url
andinvoker
properties of Loaf script attributions from thebeforeSend
function. This allow customer to scrub urls for potential PII.Changes
Refactor
limitModification
to allow arrays to be present in the path. (e.g.long_task.scripts[].source_url
will allow to modifysource_url
in long tasks (LoaF) eventsAllow modification on
long_task.scripts[].source_url
andlong_task.scripts[].invoker
Testing
I have gone over the contributing documentation.