-
Notifications
You must be signed in to change notification settings - Fork 120
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
Safer query selector #364
Safer query selector #364
Conversation
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
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.
Thank you, this looks much nicer. I notice this PR moves DOM operations from prepare
to the class constructor in a number of cases. It certainly seems less redundant, but I wonder what was the intent of splitting it like that in the first place? Are there cases where the DOM might not be ready yet in the constructor?
In general, there shouldn't be any situations where they weren't already existing, since prepare is called immediately after constructor in most cases. I think it was just as dumb design decision by me when I was doing that. I try to keep constructors free of side effects, but querySelector isn't really a side effect so I'm not sure why I did that for a bunch of things. |
This change adds a
querySelectorNotNull
function which does a null check. This is used in places where a JSDoc annotation type cast would be performed, which is generally undesirable and somewhat unsafe. While this still makes some assumptions about the actual type of theElement
, and it often casts it to something else such asHTMLElement
unsafely, it will throw if anull
is encountered.A few situations where
null
is permissible have been addressed, a few instances of ancient code have been removed, and a bunch ofquerySelector
field initializations in the settings scripts have been simplified.