Skip to content
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

Merged
merged 10 commits into from
Dec 17, 2023

Conversation

toasted-nutbread
Copy link

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 the Element, and it often casts it to something else such as HTMLElement unsafely, it will throw if a null is encountered.

A few situations where null is permissible have been addressed, a few instances of ancient code have been removed, and a bunch of querySelector field initializations in the settings scripts have been simplified.

@toasted-nutbread toasted-nutbread requested a review from a team as a code owner December 16, 2023 21:21
Copy link

github-actions bot commented Dec 16, 2023

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

Copy link
Collaborator

@djahandarie djahandarie left a 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?

@djahandarie djahandarie added this pull request to the merge queue Dec 17, 2023
Merged via the queue into themoeway:master with commit 95ad1ae Dec 17, 2023
5 checks passed
@djahandarie djahandarie added the kind/meta The issue or PR is meta label Dec 17, 2023
@toasted-nutbread
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/meta The issue or PR is meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants