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

[Bug] e.querySelectorAll(...).forEach is not a function #2099

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

eugpoloz
Copy link
Contributor

Fixes #2098.

@eugpoloz eugpoloz requested a review from a team as a code owner November 19, 2021 07:59
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 19, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9f3212f:

Sandbox Source
VKUI - default example Configuration

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2021

size-limit report 📦

Path Size
JS 58.36 KB (+0.03% 🔺)
JS, unstable 15.5 KB (0%)
CSS 29.26 KB (0%)
CSS, unstable 947 B (0%)

@@ -55,7 +55,8 @@ export const FocusTrap: React.FC<FocusTrapProps> = ({

const nodes: HTMLElement[] = [];
// eslint-disable-next-line no-restricted-properties
ref.current?.querySelectorAll(FOCUSABLE_ELEMENTS).forEach((focusableEl) => {
const allFocusableNodes = Array.from(ref.current.querySelectorAll(FOCUSABLE_ELEMENTS));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У Array.from примерно тот же уровень поддержки: https://caniuse.com/?search=Array.from
Ни IE, ни Android 4-й версии его не поддерживают

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Но мы уже используем его в других местах и он работает. Вот, например, Cell:

image

Возможно, для него у нас есть полифилл, а для NodeList.forEach нет?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нет 100% уверенности, что он работает. Может нам просто не репортят о проблемах. На счёт полифилла — я ставлю на то, что babel не полифилит ни from, ни forEach. Это можно проверить, собрав библиотеку.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Сейчас я бы использовал Array.prototype.forEach.call(ref.current.querySelectorAll(FOCUSABLE_ELEMENTS), cb)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему не пробежаться обычным for of

Copy link
Contributor

@fedorov-xyz fedorov-xyz Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему не пробежаться обычным for of

Обычный for of потом может превратиться во что-то подобное.

image

И в целом у for of не самая хорошая производительность, если сравнивать с forEach.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну раз бенчи, то вот https://jsben.ch/2uEkr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Уф давайте без бенчей это прохладный код

@github-actions
Copy link
Contributor

👀 Styleguide deployed

See the styleguide for this PR at https://vkcom.github.io/VKUI/pull/2099/

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2021

Code coverage

lines2926 / 358481.64%
statements3015 / 369881.53%
functions684 / 81983.51%
branches2338 / 312274.88%
branchesTrue0 / 0100.00%

Generated by 🚫 dangerJS against 9f3212f

@eugpoloz eugpoloz force-pushed the bug/2098-focustrap-nodelist branch from ab3395f to 9f3212f Compare November 19, 2021 09:10
@fedorov-xyz
Copy link
Contributor

А не вариант заполифиллить forEach собственно?

Например, где-нибудь здесь: https://github.com/VKCOM/VKUI/blob/master/src/lib/polyfills.ts

function makeNodeListForEach(collection) {
  if (collection && !collection.prototype.forEach) {
    if (Array.prototype.forEach) {
      collection.prototype.forEach = Array.prototype.forEach;
    } else {
      collection.prototype.forEach = function(callback, thisArg) {
        thisArg = thisArg || window;
        for (let i = 0; i < this.length; i++) {
          callback.call(thisArg, this[i], i, this);
        }
      };
    }
  }
}

makeNodeListForEach(window.NodeList);
makeNodeListForEach(window.HTMLCollection);

@eolme
Copy link
Contributor

eolme commented Nov 19, 2021

Зачем проверка на Array.prototype.forEach? Упростить можно до

if (canUseDOM) {
  window.NodeList.prototype.forEach = Array.prototype.forEach;
  window.HTMLCollection.prototype.forEach = Array.prototype.forEach;
}

@thoughtspile
Copy link
Contributor

Напомню про #1724

Глобальные полифиллы не надо больше пожалуйста

@ArthurStam
Copy link
Contributor

Я согласен с Вовой, что все эти махинации со встроенными конструкторами доведут до беды. Предлагаю юзать дедовские методы вызова методов массива на коллекции дом-элементов.

@eugpoloz eugpoloz merged commit 6b51b5f into master Nov 19, 2021
@eugpoloz eugpoloz deleted the bug/2098-focustrap-nodelist branch November 19, 2021 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] e.querySelectorAll(...).forEach is not a function
5 participants