Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: favicon.ico detection #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SaveliiLukash
Copy link

This PR fixes the false negative test result on favicon.ico(icon is actually there, test erroneously says that it is not)

Explanation:

  1. querySelectorAll always returns HTMLElement[]
  2. If the first selector does not match any elements the return value is [] (empty array)
  3. [] (empty array) is a truthy value, so [] || <second selector> will just return []
  4. As a result the second selector is never executed

Solution:
Always execute both selectors, and use their combined result

Bonus:
This will also fix the check of multiple favicon.ico files being present in case they are matched by both selectors:

} else if (icos.length > 1) {
  messages.push({
    status: CheckerStatus.Error,
    id: MessageId.multipleIcoFavicons,
    text: `There are ${icos.length} ICO favicons`
  });
}

P.S.: @phbernard Thanks a lot for your work. Have been using RealFaviconGenerator for many years!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant