-
Notifications
You must be signed in to change notification settings - Fork 805
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
feat(views): add FilteringAttributesProcessor #2733
feat(views): add FilteringAttributesProcessor #2733
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2733 +/- ##
==========================================
- Coverage 93.28% 93.27% -0.01%
==========================================
Files 158 158
Lines 5434 5443 +9
Branches 1141 1141
==========================================
+ Hits 5069 5077 +8
- Misses 365 366 +1
|
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.
LGTM with a minor suggestion
process(incoming: Attributes, _context:Context): Attributes { | ||
const filteredAttributes: Attributes = {}; | ||
for(const allowedAttributeName of this._allowedAttributeNames){ | ||
if(allowedAttributeName in incoming){ |
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 would probably prefer to use something like Object.hasOwnProperty
in order to avoid any weirdness with the object prototype. You would want to implement it respecting https://eslint.org/docs/rules/no-prototype-builtins like this maybe:
if(allowedAttributeName in incoming){ | |
if(Object.prototype.hasOwnProperty.call(incoming, allowedAttributeName)){ |
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 submitted a PR to explicitly document that only own-enumerable attribute keys count.
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 updated this in the most recent commit.
Should I also make sure that the property is enumerable, or is that something we can assume at the point where attributes are processed? 🤔
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.
Probably we can use Object.keys
to fetch the own-enumerable keys then filter them with the allowed names list and pick all of them at once. In this way, we also avoid the for-of loop since it is kinda slower than looping over an array since it depends on iterators.
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.
Thanks for the feedback. I changed it to use Object.keys
instead. 🙂
Which problem is this PR solving?
This PR is part of the work on #2592 and adds a
FilteringAttributesProcessor
, to fulfill the following spec requirement for view configuration:Short description of the changes
Adds a
FilteringAttributesProcessor
that can be passed to aView
to filter attributes.Type of change
How Has This Been Tested?
Checklist: