-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(hog): fix elements matching #24331
Conversation
Size Change: 0 B Total Size: 1.12 MB ℹ️ View Unchanged
|
5748f4c
to
a4a4e14
Compare
This reverts commit bf0937c.
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
plugin-server/src/cdp/utils.ts
Outdated
if (elementsChain) { | ||
Object.defineProperties(response, { | ||
elements_chain_href: { | ||
get: () => getElementsChainHref(elementsChain), |
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.
Love this solution. I'm wondering though if we should also cache these values? Maybe not worth it for now...
Also a comment here would be helpful for future travellers to understand what this is all for (I barely get it right now)
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.
Also its not awesome that there is only a test for one path...
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 I found a rather neat way to cache this: e2e1bc7
More tests coming soon...
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.
wooohhhhhh self destructing function :o
Ready for a re-review |
Problem
It started from this:
This exposes two problems:
elements_chain_*
columns in Hog globalsarrayExists
(solved)Changes
ParsedClichouseEvent
was not used anywhere 🤷)elements_chain_href
,elements_chain_texts
,elements_chain_ids
,elements_chain_elements
into Hog filter matching.indexOf
,position
,positionCaseInsensitive
andarrayCount
Extra
HogFunctionInvocationGlobals
fieldevent
, in order to better match the actual database schema. However since it has more discrepancies (name
instead ofevent
for the actual event, etc), I decided to leave it for now.Alternatives considered
event
instead of lazily getting them. Rejected as this will be wasteful if you have no functions that look for element textsHow did you test this code?
Added a test in
hog-executor.test.ts
, poked around in the UI.